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

Fix simple key handling #1031

Closed
wants to merge 1 commit into from
Closed

Fix simple key handling #1031

wants to merge 1 commit into from

Conversation

DMaxter
Copy link

@DMaxter DMaxter commented Apr 14, 2024

Currently if a key is more than 128 characters, it is handled as a complex mapping key, however this goes against the YAML spec as pointed out in #849 (comment) . Can be read here: https://yaml.org/spec/1.2.2/#flow-mappings

This causes issues when handling OpenAPI specifications with paths longer than 128 characters, making them invalid.

This PR aims to fix this issue, which is also affecting yq as can be seen here mikefarah/yq#1192

Fixes #849

@AntoineJac
Copy link

Does anyone know if this PR will accepted and the release version for this feature? Thanks

@DMaxter
Copy link
Author

DMaxter commented Jun 3, 2024

@AntoineJac I contacted the maintainer via email but got no answer. This project seems completely abandoned. But worth tagging @niemeyer , maybe we are lucky

@niemeyer
Copy link
Contributor

niemeyer commented Jun 3, 2024

Sorry, I'm still here. I just haven't had the time to provide feedback and work on the project that is due. As I said elsewhere, I want to get a team to be responsible for this so it's not a one man show, but I've been failing to allocate that.

As far as this PR is concerned, it seems reasonable, but it needs:

  1. Assessment regarding backwards compatibility
  2. Tests
  3. Some comments about whether unicode is being handled correctly (the spec talks about unicode, not bytes)

@Tieske
Copy link

Tieske commented Jun 25, 2024

tried to fix this in #1037

@DMaxter
Copy link
Author

DMaxter commented Jun 25, 2024

Thank you @Tieske I think it is sensible to close this PR in favor of yours, since yours is more complete!

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.

Key size in dicts seems to be limited to 128 characters
4 participants