Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Move ratings client to monorepo #1505

Merged
merged 7 commits into from
Nov 28, 2023

Conversation

spydon
Copy link
Collaborator

@spydon spydon commented Nov 24, 2023

No description provided.

@matthew-hagemann
Copy link
Collaborator

@spydon Looks good! I have made some changes to the client that need to be reviewed and then included for the chart endpoints 🙂

Copy link
Collaborator

@matthew-hagemann matthew-hagemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎸 just one small question on naming.

@@ -0,0 +1,22 @@
name: app_center_ratings_client
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you renamed the main dart file to ratings_client.dart . Should we not then update the name in the pubspec.yaml to reflect that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ratings_client.dart is only a barrel file, so once it is imported it will say something like:

import 'package:app_center_ratings_client/ratings_client.dart';

instead of the slightly over-kill:

import 'package:app_center_ratings_client/app_center_ratings_client.dart';

We could rename the whole package, but maybe it is good to state in the name what the ratings are for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it is actually not a barrel file only, but same same.
The class in there is called RatingsClient, so then the file should be called that (apparently we haven't turned on that lint rule). :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably should be a barrel file though, I'll update the PR to that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, we're back to app_center_ratings_client.dart, because otherwise it will have the same name as the now new src/ratings_client.dart. Good talk. 😂

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, I feel left out now 😂

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have it in the back of my head I renamed it to match the name in the pubspec because of a rule re: uploading to pub.dev? I could be mistaken

@spydon spydon merged commit 0fdb5f4 into ubuntu:main Nov 28, 2023
8 checks passed
ashuntu pushed a commit to ashuntu/app-center that referenced this pull request Feb 28, 2024
…orepo

chore: Move ratings client to monorepo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants