Skip to content
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

Remove defaults and stop outputting fake data #88

Merged
merged 3 commits into from
Nov 15, 2021

Conversation

movermeyer
Copy link
Collaborator

@movermeyer movermeyer commented Nov 13, 2021

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.dtd schema), so there is no way they could be missing.

In the one case where the attribute is not a required attribute (type on dateTimeFormatLength), it was defaulting to a value that is not valid (:format). Once again, there are no current instances of a dateTimeFormatLength without a type attribute. 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 root where <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

  • Gets rid of defaults that would be dangerous if they were used.
  • Gets rid two incorrect data from root as a result of not following aliases.

Testing

You could run thor cldr:export for master and this branch, then run diff -r to compare the outputs:

git checkout master
thor cldr:export --target=./data_from_master

git checkout movermeyer:mo.stop_outputting_fake_data
thor cldr:export --target=./data_from_pr

diff -r data_from_master data_from_pr

Which outputs:

diff -r data_from_master/root/calendars.yml data_from_pr/root/calendars.yml
81,84d80
<         name:
<           0: ''
<         narrow:
<           0: ''

@movermeyer movermeyer force-pushed the mo.stop_outputting_fake_data branch 2 times, most recently from 9a1d084 to 1e5b3fd Compare November 13, 2021 22:53
@movermeyer movermeyer marked this pull request as ready for review November 13, 2021 23:04
@movermeyer movermeyer marked this pull request as draft November 15, 2021 14:40
@movermeyer movermeyer force-pushed the mo.stop_outputting_fake_data branch from 1e5b3fd to 045c7d3 Compare November 15, 2021 14:49
@movermeyer movermeyer marked this pull request as ready for review November 15, 2021 14:58
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.
@movermeyer movermeyer force-pushed the mo.stop_outputting_fake_data branch 4 times, most recently from 5de429f to ef8dcd8 Compare November 15, 2021 17:51
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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

`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.
@movermeyer movermeyer force-pushed the mo.stop_outputting_fake_data branch from ef8dcd8 to 407e87c Compare November 15, 2021 20:07
@Korri Korri merged commit 98ea81a into ruby-i18n:master Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stop defaulting values incorrectly
2 participants