[UoM prototype] Import measurement attribute vocabulary (2/4) - #951
[UoM prototype] Import measurement attribute vocabulary (2/4)#951ricardotejedorsanz wants to merge 1 commit into
Conversation
| @@ -44,9 +44,15 @@ en: | |||
| absinthe_variety: | |||
| name: Absinthe variety | |||
| description: Differentiates absinthe products by their unique varieties, e.g. rouge, verte | |||
| absorbency_capacity: | |||
| name: Absorbency capacity | |||
There was a problem hiding this comment.
Localization quality issue found
The following issues may affect the quality of localized translations if they are not addressed:
- The value
Absorbency capacityfor keyen.attributes.absorbency_capacity.nameis very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.
Please look out for other instances of this issue in your PR and fix them as well if possible.
Questions about these messages? Hop in the #help-localization Slack channel.
| @@ -44,9 +44,15 @@ en: | |||
| absinthe_variety: | |||
| name: Absinthe variety | |||
| description: Differentiates absinthe products by their unique varieties, e.g. rouge, verte | |||
| absorbency_capacity: | |||
| name: Absorbency capacity | |||
| description: Absorbency capacity | |||
There was a problem hiding this comment.
Localization quality issue found
The following issues may affect the quality of localized translations if they are not addressed:
- The value
Absorbency capacityfor keyen.attributes.absorbency_capacity.descriptionis very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.
Please look out for other instances of this issue in your PR and fix them as well if possible.
Questions about these messages? Hop in the #help-localization Slack channel.
| @@ -161,6 +182,12 @@ en: | |||
| adc_resolution: | |||
| name: ADC resolution | |||
| description: Specifies the resolution (in bits) of the integrated Analog-to-Digital Converter, defining measurement precision | |||
| added_length: | |||
| name: Added length | |||
There was a problem hiding this comment.
Localization quality issue found
The following issues may affect the quality of localized translations if they are not addressed:
- The value
Added lengthfor keyen.attributes.added_length.nameis very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.
Please look out for other instances of this issue in your PR and fix them as well if possible.
Questions about these messages? Hop in the #help-localization Slack channel.
jeanguo-png
left a comment
There was a problem hiding this comment.
Non-blocking: since this PR compacted taxonomy.json to stay under GitHub's 100MB file limit, should we also add a small guard for categories.json? It is already ~95.7MB after this import, so the next assignment growth could hit the same limit.
danielpgross
left a comment
There was a problem hiding this comment.
Regarding the 100MB limit: we ran into the same challenge in the most recent release and our solution there was to move the file to LFS -- I think that's what we want here too. This isn't ideal long-term though, because it means the raw file can't be fetched without following an LFS pointer. Since these dist files are meant for easy consumption via API and the Github UI, we should think about a better long-term approach for that. But in the meantime, LFS is the way to go IMO.
As for the PR changes, my main question is about attributes like:
- adapter_case_interior_height
- bag_case_interior_height
- bowling_pin_height
- carpet_pile_height
Intuitively, I expected these to be extended attributes of a height base attribute, but I realized that they're actually each base attributes. I understand that this is necessary because they can support different units, but it still feels a bit incongruent with the existing attribute data model. I wonder if we can somehow make the base/extended attribute concept work here too. If not, should we somehow group all _height attributes together? Right now, they're just grouped in under dimension with others like width, length, depth, diameter, etc.
ab2fed9 to
9719167
Compare
70e5460 to
46b2ef5
Compare
9719167 to
34009f4
Compare
46b2ef5 to
3e9cc33
Compare
Good callout @danielpgross. I agree there is a semantic relationship between many of these height-like attributes, but I’m not sure extended attributes give us the same benefit for measurement attributes that they give us for closed-list attributes. For closed-list attributes, the main benefit is inheriting the same controlled value set from the base attribute. For measurement attributes, the inherited contract is For family-style groupings like |
Good point @jeanguo-png, Daniel noticed this too and is looking at it separately, so I’m going to keep that out of this MVP import PR (with a proposed fix is to stop storing generated |
What this PR does
This PR imports the Core/Admin-compatible V0 set of measurement attributes into the public taxonomy package.
taxonomy.jsonformattingThis PR also changes
dist/en/taxonomy.jsonto be generated in compact JSON form. This is intentional, but it is not a trivial formatting change: after adding the measurement vocabulary, the pretty-printed generatedtaxonomy.jsonis ~106 MB, which exceeds GitHub's hard 100 MB per-file limit and cannot be pushed.The compact file preserves the same JSON data contract for consumers that parse
taxonomy.json, but it does make the file much less human-readable and creates a large formatting-only diff for that generated file. This PR still contains real taxonomy data additions; the formatting-only note applies specifically to the large line churn indist/en/taxonomy.json. Reviewers should treat the large deletion count indist/en/taxonomy.jsonas a consequence of compacting that generated file, not as deleted taxonomy content.This is acceptable for prototype purposes, but we should still explore the right long-term distribution contract for large generated taxonomy artifacts.
What is included
This PR imports:
1,235public measurement attributes total33,219category/measurement assignmentsdist/enfilesFiles changed
data/attributes.ymltype: measurement,measurement_type, andsupported_unitsdata/categories/*.ymldata/localizations/attributes/en.ymldist/en/attributes.jsonanddist/en/attributes.txtdist/en/categories.jsonanddist/en/taxonomy.jsondev/lib/product_taxonomy/commands/generate_dist_command.rbtaxonomy.jsonin compact JSON form so the generated taxonomy distribution stays below GitHub's file size limit as measurement coverage growsdev/test/integration/all_data_files_import_test.rbUnit representation
For V0,
supported_unitscontains canonical taxonomy unit symbols such ascm,in,ml, andfl oz.Using symbols keeps the taxonomy data intentionally compact and compatible with other locales. The taxonomy should define the measurement vocabulary: which attributes exist, which categories they apply to, which measurement type they use, and which unit symbols are supported for each attribute.
This keeps the public taxonomy contract simple for V0. The remaining integration details—mapping taxonomy unit symbols to Core-supported units, validating supported units for each measurement type, and normalizing inferred or predicted measurement values before persistence—will be addressed next in Core as part of its own technical design.
Validation
bundle exec rake test:unitbundle exec rake test:integrationbundle exec rake schema:vet