-
Notifications
You must be signed in to change notification settings - Fork 54
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
stregreport: Add all categories to overview #532
Conversation
Removed product IDs, and removed the wet HTML with templating.
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.
there's a few small questions, descide for yourself whether you want to fix any of them, but i don't think there's anything wrong with the current code
good change 👍
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.
Looks good to me
@krestenlaust i've just tried running it before my last review and code now, it seems the categories doesn't show properly for me using the testdata? same testdata: testdata.json |
This reverts commit aac3ee0.
b5dfc45
to
735e9b1
Compare
After bisecting, it turns out it was the 'pythonic' implementation that was faulty. I ended up simply reverting it, because it (to me) is actually harder to understand than the current imperative. Let me know what you think I've implemented a test to prevent this sort of outfalling again |
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.
lgtm, works on my machine 👍
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #532 +/- ##
==========================================
+ Coverage 80.67% 81.63% +0.96%
==========================================
Files 40 40
Lines 3886 3899 +13
Branches 485 485
==========================================
+ Hits 3135 3183 +48
+ Misses 694 656 -38
- Partials 57 60 +3 ☔ View full report in Codecov by Sentry. |
The current implementation is very crude and hardcodes the product IDs.
Instead we should use the categories, and make it dynamic.
Fixes #514
I've also renamed the stregreport menu-entries to more reasonable names