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

feat: adding support for getChart #9

Merged
merged 3 commits into from
Nov 28, 2023

Conversation

matthew-hagemann
Copy link
Collaborator

@matthew-hagemann matthew-hagemann commented Nov 24, 2023

Implementing getChart gRPC

Ratings service PR

@matthew-hagemann
Copy link
Collaborator Author

matthew-hagemann commented Nov 27, 2023

Not letting me add non-Canonical members as reviewers, but here are the changes to the client for the upcoming chart endpoints @spydon @BLKKKBVSIK

@@ -3,15 +3,18 @@ import 'dart:async';
import 'package:grpc/grpc.dart';
import 'package:meta/meta.dart';

import 'src/app.dart' as app;
import 'src/chart.dart' as chart;
Copy link

Choose a reason for hiding this comment

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

Does this contain classes with name collisions with other imports? Otherwise I'd drop the as chart.

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 is defined in my code and in the generated code, thats why I gave a prefix to mine. If it makes it clearer, I could swap the prefix to the protobuf generated classes and prefix them with pb.?:
package:app_center_ratings_client/src/chart.dart
package:app_center_ratings_client/src/generated/ratings_features_chart.pb.dart

Copy link

Choose a reason for hiding this comment

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

Ah, that would probably be clearer indeed!


part 'app.freezed.dart';
@immutable
class Rating {
Copy link

Choose a reason for hiding this comment

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

Since there are only Rating related things in here I would name this file rating.dart or something like that, I usually try to avoid file names like common or utils, since they don't really say much about the content (and also allows for a wide mix of things within one file, which is usually discouraged).

@matthew-hagemann matthew-hagemann merged commit 86efa7f into canonical:main Nov 28, 2023
4 checks passed
@matthew-hagemann matthew-hagemann deleted the chart-endpoint branch November 28, 2023 12:28
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