feat: packaging stats per parent materials #8594
Conversation
github-actions
bot
added
🌱 Green-Score
github-actions
bot
added
API
|
@stephanegigandet could you add a comparison to category ? |
|
@stephanegigandet could you swap column A and column B ? |
Well it will be the next step, we first need to aggregate by product, then we can aggregate by category, and we can show a comparison to the category on each product. It will be much more work though. |
Sure, done. |
Codecov Report
@@ Coverage Diff @@
## main #8594 +/- ##
==========================================
+ Coverage 48.69% 48.79% +0.09%
==========================================
Files 114 114
Lines 21417 21466 +49
Branches 4791 4803 +12
==========================================
+ Hits 10430 10474 +44
- Misses 9701 9703 +2
- Partials 1286 1289 +3
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
lib/ProductOpener/Packaging.pm
Outdated
| foreach my $parent ("en:paper-or-cardboard", "en:plastic", "en:glass", "en:metal") { | ||
| if (is_a("packaging_materials", $material, $parent)) { | ||
| $parent_material = $parent; | ||
| last; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
It's okay, but don't we have a more efficient way to do this ? (like fetching all parents and then computing intersection with our values ?). Maybe with something like Set::Tiny.
There was a problem hiding this comment.
I don't know enough of the source code to suggest an entirely different approach to the logic itself, but a common pattern for getting the first item in a list that matches a certain criteria is using List::Util's first:
use List::Util qw(first);
$parent_material = first {
is_a("packaging_materials", $material, $_)
} ("en:paper-or-cardboard", "en:plastic", "en:glass", "en:metal");but if this code is hot (i.e. running several times per call) maybe it could be worth turning is_a() into a hash lookup.
Either way, the current implementation is okay and I wouldn't change it unless it was a bottleneck.
There was a problem hiding this comment.
Thanks for the suggestions. I changed to @garu 's suggestion as I find it more readable with first indeed.
It certainly could be done more efficiently, but this code is executed only when writing products, and given the shallowness of the packaging materials taxonomy, it's just a dozen operations, so I'd prefer not to spend time on it. If we want to optimize things, there are targets that are much more likely to make a difference (e.g. for read operations of products, where we compute attributes and knowledge panels for 100 products...)
| [% END %] | ||
| "html": ` | ||
| [% FOREACH packaging IN product.packagings %] | ||
| [% IF packaging.recycling == recycling_type OR (recycling_type == "en:unknown" AND ((NOT packaging.recycling.defined) OR ((packaging.recycling != "en:discard") AND (packaging.recycling != "en:recycle")))) %] |
There was a problem hiding this comment.
A better way to do that, would have been to dispatch packagings in arrays in recycling_types instead of putting => 1. So that you just have to iterate over the hash, and elements in array.
Because this condition is not trivial to read !
There was a problem hiding this comment.
Agreed, but it's likely that we will change how we display packaging components in the future. This code did not change, I just renamed the template for clarity.
| 'en:plastic' => {} | ||
| } | ||
|
|
||
| ) or diag explain $product_ref->{packagings_materials}; |
There was a problem hiding this comment.
Would be good to add at least one test with an unknown material.
I always like to also test with an empty hash.
Co-authored-by: Alex Garel <[email protected]>
…s.tt.json Co-authored-by: Alex Garel <[email protected]>
|
Kudos, SonarCloud Quality Gate passed!
|
This PR:
Part of