Skip to content

Fix local file changes on "make normalize_yaml"#5028

Closed
aduth wants to merge 14 commits intomainfrom
aduth-normalize-yaml-avoid-local-changes
Closed

Fix local file changes on "make normalize_yaml"#5028
aduth wants to merge 14 commits intomainfrom
aduth-normalize-yaml-avoid-local-changes

Conversation

@aduth
Copy link
Copy Markdown
Contributor

@aduth aduth commented May 5, 2021

Why: To prevent accidental changes, to bring CircleCI build and local development into stricter alignment, to ensure that make normalize_yaml actually normalizes the way that we'd expect it to, and to expand test coverage for YAML normalization to cover all files formatted by the Make target.

Initially I'd planned for the month_names change to set null as the first entry, but it appears that YAML.dump formats this back to the empty entry. Not sure if this produces inconsistent results in other environments.

If we want to retain the comments in country_dialing_codes.yml, would there be a reasonable alternative place to put them? Or would it suffice to reference git blame to recall the reason for a setting?

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@mitchellhenke mitchellhenke left a comment

Choose a reason for hiding this comment

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

👍🏼 👍🏼 👍🏼 👍🏼

@aduth
Copy link
Copy Markdown
Contributor Author

aduth commented May 10, 2021

Update: I was having trouble last week with CircleCI vs. locally behaving differently. In the meantime, I discovered a fix for the issue I'd had with getting the JavaScript/Prettier reimplementation to sort keys, so I may explore that route again to both circumvent the CircleCI issues and actually support comments in the YAML. Hoping to revisit this week, but considering it as lower priority than other tasks.

aduth and others added 12 commits May 13, 2021 13:31
**Why**: So there's no inconsistency between local environments and CircleCI test results
**Why**: Differences in behavior of normalize_yaml seem to be associated with a bug in libyaml which is expected to have been resolved in 0.2.5. Maybe the new pre-built images have it.
**Why**: To get new features and bug fixes, and to avoid needing to work around this in CI to manually downgrade to an older version.
Resolve CircleCI error "make: i18n-tasks: Command not found"
**Why**: Errors seen previously were likely caused by issue fixed in 776a9fa8e
Causing errors in CircleCI "cannot load such file". Ideally we'd diagnose why those errors are happening, but AFAICT the dependency isn't used anyways?
**Why**: Should always go through the Makefile script, which includes additional formatting passes
@aduth aduth force-pushed the aduth-normalize-yaml-avoid-local-changes branch from 061ffbc to df0c06c Compare May 13, 2021 17:34
@aduth
Copy link
Copy Markdown
Contributor Author

aduth commented May 17, 2021

Closing in favor of #5066

@aduth aduth closed this May 17, 2021
@aduth aduth deleted the aduth-normalize-yaml-avoid-local-changes branch May 17, 2021 20:11
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.

3 participants