-
-
Notifications
You must be signed in to change notification settings - Fork 72
feat: Add flexibility to PriceTotalStats #1059
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: Add flexibility to PriceTotalStats #1059
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @AshutoshKhadse23!
As you may have already understood from openfoodfacts/smooth-app#6553, your solution is a bit too complex.
Let's focus on openfoodfacts/smooth-app#6553 first.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1059 +/- ##
==========================================
- Coverage 76.34% 75.54% -0.81%
==========================================
Files 239 264 +25
Lines 8494 10031 +1537
==========================================
+ Hits 6485 7578 +1093
- Misses 2009 2453 +444 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@monsieurtanuki , |
@AshutoshKhadse23 Good news: we don't need |
@monsieurtanuki , |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @AshutoshKhadse23!
The point of the issue was to provide flexibility, and with a visible json
field we have all the flexibility we need.
The getInt
method makes sense too, as it's frequently used.
My initial thought was to use constants (or methods) as tags, and no getters. Another solution is to implement all getters.
You implemented both, with is more than enough.
I think we'd be better off with the getters and without the constants.
What do you think of it?
@monsieurtanuki, I agree with you and changed the code with the getters and without the constants. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @AshutoshKhadse23, and congratulations for your first PR in this specific project!
Closes: #1058
Added all 50 fields.
And provide the flexibility.
This is how it will look :