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: getChart #41

Merged
merged 10 commits into from
Jan 8, 2024
Merged

feat: getChart #41

merged 10 commits into from
Jan 8, 2024

Conversation

matthew-hagemann
Copy link
Collaborator

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

  • Updated ChartData to include the ratings_band and raw rating
  • Removed chart types
  • Refactoring protos to make Ratings common and reusable across features/app & features/chart
  • Refactoring entities to make Rating and RatingsBand reusable
  • Refactoring db methods to use VoteSummary instead of extracting a list of votes and processing in Rust
  • Implementing getChart endpoint
  • e2e tests now run in ci

Closes #35, #15

Client update

@matthew-hagemann matthew-hagemann changed the title feat: updating chart protos feat: getChart Nov 27, 2023
@matthew-hagemann matthew-hagemann force-pushed the chart-protos branch 2 times, most recently from 81cb188 to dc8f5e7 Compare November 28, 2023 12:47
@@ -1,16 +1,11 @@
use crate::app::AppContext;

use self::protobuf::{App, GetRatingRequest, GetRatingResponse};
pub use protobuf::app_server;
use crate::features::pb::app::{GetRatingRequest, GetRatingResponse};
Copy link
Contributor

Choose a reason for hiding this comment

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

This also applies to Line 1 and later lines, but probably best to merge all the use crate::{...} into a single line so it's easier to find all imports.


tonic::include_proto!("ratings.features.app");
}
use super::{service::AppService, use_cases};
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to not use crate:: here? I rarely see Rust crates use super except in unit tests, but I won't be too picky.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No reason, totally agree with use crate:: 🙂

@@ -1,7 +1,7 @@
use crate::app::AppContext;
use crate::{app::AppContext, features::common::entities::Rating};
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe merge with the super block below (as per previous comment)

@ZoopOTheGoop ZoopOTheGoop merged commit d8508f8 into ubuntu:dev Jan 8, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Top charts endpoint
3 participants