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

Refresh the JSON specs #113

Merged
merged 2 commits into from
Mar 9, 2021
Merged

Refresh the JSON specs #113

merged 2 commits into from
Mar 9, 2021

Conversation

gasche
Copy link
Contributor

@gasche gasche commented Dec 24, 2020

This PR contains two changes:

  • A minor change to the Rakebuild file that was necessary, on my machine, to get rake build to render JSON specs properly (otherwise the ATTN warning would be at the end of each test, instead of at the begining as in their current versioned form.)
  • A new run of rake build that refreshes .json files that are stale compared to the .yml reference.

… after

On my system (yaml gem version 0.1.0), `foo.merge(bar)` will include the
keys of `foo` before the keys of `bar`, so the previous Rakebuild
definition would include the ATTN warning *after* the test data in the
rendered JSON files.
@gasche
Copy link
Contributor Author

gasche commented Jan 9, 2021

I now realize that this PR overlaps with #90 from August 2015 which is doing the same thing. This should have been fixed long ago.

@spullara
Copy link
Collaborator

spullara commented Mar 9, 2021

Any reason not to merge this?

@gasche
Copy link
Contributor Author

gasche commented Mar 9, 2021

I would be in favor of merging, indeed!

I don't think it is necessary, but before merging you could maybe try to see if you can reproduce the warning-placement issue on your system: if you run the command to refresh the specs (rake build) on the current main branch, do you see the warning as the first key of the generated JSON objects, as intended, or do you observe a difference placement as on my machine?

@Danappelxx
Copy link
Collaborator

Confirming the warning-placement issue, thanks for fixing. Agree, should have been merged a long time ago.

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