-
-
Notifications
You must be signed in to change notification settings - Fork 407
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: dq new facet for opposite tags #10378
Conversation
Error with unit test attributes.t, and more particularly en-attributes.json. The file is initially (expected_test_results):
After changes from this PR it becomes
This is definitely related to the PR because fair-trade and organic both contains opposites. |
|
From data quality meeting minutes:
-> done
-> done
-> used incompatible_with instead, see below
-> done
-> replace this similar error by the present one would imply to add incompatible_with for all vegan/vegetarian ingredients. Instead, keeping this similar error - using the vegan:en:yes tag - seems to be fine. CommentsCouple of typos from previous PR #10392
Can we replace opposite tags in the taxonomy (labels) by incompatible_with (+ update Tags.pm)? (question for @stephanegigandet maybe, this tag existed already before the present PR) |
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.
Great PR, I have small suggestions.
|
||
For each tag of a given field ($tagtype, can be "labels" or "categories", for example), | ||
and a given property ($prop_name, without last column (:). Can be "incompatible_with:en", for example), | ||
return a hash of tagid <-> property_value |
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.
I don't know if it's what's wanted, but it does not handle property inheritance and we have to document it !
return a hash of tagid <-> property_value | |
return a hash of tagid <-> property_value | |
Note that this does not handle inheritance. |
lib/ProductOpener/DataQualityFood.pm
Outdated
sub check_incompatible_tags ($product_ref) { | ||
|
||
# list of tags having 'incompatible_with' properties | ||
my @tags_to_check = ("categories", "labels"); |
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.
tagtypes_to_check would be less error prone.
lib/ProductOpener/DataQualityFood.pm
Outdated
# list of tags having 'incompatible_with' properties | ||
my @tags_to_check = ("categories", "labels"); | ||
|
||
foreach my $tag_to_check (@tags_to_check) { |
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.
here also $tagtype_to_check.
lib/ProductOpener/DataQualityFood.pm
Outdated
foreach my $key (keys %{$incompatible_with_hash}) { | ||
my $value = %{$incompatible_with_hash}{$key}; |
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.
I would have named: $key --> $tag and $value -> $incompatible_tags
It makes it easier to read.
lib/ProductOpener/DataQualityFood.pm
Outdated
my @incompatible_tags = sort ($tag_to_check . "-" . $key, $incompatible_tag); | ||
|
||
add_tag($product_ref, "data_quality_errors", | ||
"en:mutually-exclusive-$incompatible_tags[0]-and-$incompatible_tags[1]"); |
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.
Maybe mutually-exclusive-tags- as prefix ?
"en:mutually-exclusive-$incompatible_tags[0]-and-$incompatible_tags[1]"); | |
"en:mutually-exclusive-tags-$incompatible_tags[0]-and-$incompatible_tags[1]"); |
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.
Shan't we also add a "en:mutually-exclusive-tags" tag, to be able to get all items with incompatibilities ?
lib/ProductOpener/DataQualityFood.pm
Outdated
foreach my $tag_to_check (@tags_to_check) { | ||
$log->debug("check_incompatible_tags: tag_to_check $tag_to_check") if $log->debug(); | ||
|
||
my $incompatible_with_hash = get_all_tags_having_property($product_ref, $tag_to_check, "incompatible_with:en"); |
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.
my $incompatible_with_hash = get_all_tags_having_property($product_ref, $tag_to_check, "incompatible_with:en"); | |
# we don't need to care about inherited properties | |
# as every tag parent is also in the _tags field | |
# thus incompatibilities will pop-up | |
my $incompatible_with_hash = get_all_tags_having_property($product_ref, $tag_to_check, "incompatible_with:en"); |
Thanks for the review and the suggestions. This is still a problem: #10378 (comment) |
lib/ProductOpener/Tags.pm
Outdated
|
||
sub get_all_tags_having_property ($product_ref, $tagtype, $prop_name) { | ||
my %tag_property_hash = (); | ||
foreach my $tagid (@{$product_ref->{$tagtype . "_tags"}}) { |
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.
This will create en empty labels_tags / categories_tags array if it did not exist.
In Attributes.pm, if there's no labels_tags, then we consider we don't know if a product has the organic label or not. But if it has other labels (e.g. fair trade), then we consider that it if were organic, we would have the organic label too.
There's a bug in Attributes.pm that results in an empty labels_tags array being considered as having labels.
|
remove opposite when not needed (categories) put back opposite when it was used, before #10378 (labels). Remember that opposite tag is used in Import.pm.
What
Added new facet for opposite tags in labels and categories taxonomies.
*Remark: facets don't contain language code "en:" because otherwise the description will not work.
Added new function in Tags.pm. I thought it might be useful for other use cases.
Screenshot
Related issue(s) and discussion