Remove defaults and stop outputting fake data#88
Merged
Korri merged 3 commits intoruby-i18n:masterfrom Nov 15, 2021
Merged
Conversation
9a1d084 to
1e5b3fd
Compare
1e5b3fd to
045c7d3
Compare
This attribute is required by the `ldml.dtd`, and indeed, there is no case in the data that is missing that attribute
`:format` is not a valid `type` anyway, so it would never match anyway. In the `ldml.dtd`: * For `dateFormatLength`, `timeFormatLength` the `type` attribute is `#REQUIRED`. * For `dateTimeFormatLength`, `type` attribute is `#IMPLIED` There are no instances in the code where any of these are missing the `type` attribute. So while in the future it could technically happen for `dateTimeFormatLength`, I'd rather the code fail in that case, then silently pass with a nonsense value.
5de429f to
ef8dcd8
Compare
movermeyer
commented
Nov 15, 2021
| result[key] = select(path).inject({}) do |ret, node| | ||
| type = node.attribute('type').value.to_i rescue 0 | ||
| key_result = select(path).inject({}) do |ret, node| | ||
| next ret if node.name == "alias" # TODO: Actually handle alias nodes, https://github.com/ruby-i18n/ruby-cldr/issues/78 |
Collaborator
Author
There was a problem hiding this comment.
The root locale contains <alias> nodes for calendar eras:
These are incorrectly getting output as:
eras:
abbr:
0: BCE
1: CE
name:
0: ''
narrow:
0: ''Until #78 is addressed, I figure it is better not to output any data, rather than output incorrect data.
Korri
approved these changes
Nov 15, 2021
`type` is a required attribute of `era` tags.
`<alias>` tags are present in the `:root` locale and were incorrectly getting output as:
```yaml
eras:
abbr:
0: BCE
1: CE
name:
0: ''
narrow:
0: ''
```
Until ruby-i18n#78 is addressed, I figure it is better not to output any data, rather than incorrect data.
ef8dcd8 to
407e87c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What are you trying to accomplish?
Fixes #87. Removing defaults that would be incorrect if used.
What approach did you choose and why?
These commits remove unnecessary defaults. There are no instances of these attributes missing in the CLDR data, so there is no impact on the data exported by
ruby-cldr.In all but one case, the attributes are already required attributes (in the
ldml.dtdschema), so there is no way they could be missing.In the one case where the attribute is not a required attribute (
typeondateTimeFormatLength), it was defaulting to a value that is not valid (:format). Once again, there are no current instances of adateTimeFormatLengthwithout atypeattribute. In the rare case there were to ever be one, I'd rather have the export fail than silently pass with an invalid default value.There are two instances in
rootwhere<alias>nodes were being handled incorrectly. Those keys are removed from the output by this change. See this comment for explanation.What should reviewers focus on?
🤷
Note that this doesn't solve the problem of mixing number systems (#89).
The impact of these changes
rootas a result of not following aliases.Testing
You could run
thor cldr:exportformasterand this branch, then rundiff -rto compare the outputs:Which outputs: