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.

Sign up for GitHub

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

Jump to bottom

Switch from serde_yaml to serde_yml #14630

Merged
merged 2 commits into from
Dec 25, 2024
Merged

Switch from serde_yaml to serde_yml #14630

merged 2 commits into from
Dec 25, 2024

Conversation

rikukiix
Copy link
Contributor

Description

This PR fixes #14339.

Since serde_yaml is already deprecated, replaced it with serde_yml.

After this change, the to yaml boolean parsing issue in #14339 is also fixed.
Now the command

['y' 'Y' 'yes' 'Yes' 'YES' 'n' 'N' 'no' 'No' 'No' 'on' 'On' 'ON' 'off' 'Off' 'OFF'] | to yaml

will return

- 'y'
- 'Y'
- 'yes'
- 'Yes'
- 'YES'
- 'n'
- 'N'
- 'no'
- 'No'
- 'No'
- 'on'
- 'On'
- 'ON'
- 'off'
- 'Off'
- 'OFF'

User-Facing Changes

I'm not sure if the yaml spec change is a user-facing change.

Tests + Formatting

  • cargo fmt --all
  • cargo clippy --workspace -- -D warnings -D clippy::unwrap_used
  • cargo test --workspace
  • cargo run -- -c "use toolkit.nu; toolkit test stdlib"

After Submitting

@fdncred fdncred added dependencies Pull requests that update a dependency file wait-until-after-nushell-release labels Dec 19, 2024
@sholderbach
Copy link
Member

Thanks for going through that process!

There may be a concern with serde_yml that they don't seem to have an issue tracker set up (yet): sebastienrousseau/serde_yml#34

A change in the behavior of a script due to a different yaml parse would be a user facing change so we should report them in the release notes. (although in this case unlikely breaking intended behavior as it sounds more like a compatibility thing for going into older 1.1 parsers according to #14339)

For completeness sake would you mind adding a test with this edge case, so we notice a regression should we migrate to another library in the future or roll our own implementation.

@rikukiix
Copy link
Contributor Author

For completeness sake would you mind adding a test with this edge case, so we notice a regression should we migrate to another library in the future or roll our own implementation.

Sure, I'll add it soon.

@WindSoilder WindSoilder merged commit 6ebc0fc into nushell:main Dec 25, 2024
15 checks passed
@github-actions github-actions bot added this to the v0.102.0 milestone Dec 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file wait-until-after-nushell-release
Projects
None yet
Milestone
v0.102.0
Development

Successfully merging this pull request may close these issues.

to yaml - use quotes for strings that will be parsed to boolean (avoid problems with yaml 1.1 spec parsers
4 participants
@rikukiix @sholderbach @fdncred @WindSoilder