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

[deps] Update serde_yaml to 0.9 #8320

Closed
aborg-dev opened this issue Jan 10, 2023 · 1 comment
Closed

[deps] Update serde_yaml to 0.9 #8320

aborg-dev opened this issue Jan 10, 2023 · 1 comment
Assignees
Labels
C-housekeeping Category: Refactoring, cleanups, code quality

Comments

@aborg-dev
Copy link
Contributor

At the moment we depend on serde_yaml version "0.8.16". This version is known to have a few bugs when parsing invalid YAML files that were triggered by #8310:

  • Parsing empty file returns an error instead of empty YAML map
  • Parsing a map with empty key does not return an error and instead yields a map with empty string being a key

Both of these bugs are non-critical for real usage and could be fixed with additional checks, but this complicates the code. This issue tracks the need to update the version of serde_yaml and remove these workarounds (that will be marked with TODOs in the code).

At the moment updating serde_yaml to 0.9 is blocked on two crates:

  • paperclip: I've updated it to latest 0.7.1 version that depends on serde_yaml 0.9 according to Cargo.toml, but on Cargo it still depends on the serde_yaml 0.8. This should be a matter of raising an issue in the project to update a package in Cargo.
  • insta: we need at least "1.18.0", but updating to this version results in many other version conflicts in dependencies and breaks the build due to changed interface, so this requires some migration work
@aborg-dev aborg-dev self-assigned this Jan 10, 2023
@aborg-dev aborg-dev added the C-housekeeping Category: Refactoring, cleanups, code quality label Jan 10, 2023
near-bulldozer bot pushed a commit that referenced this issue Jan 19, 2023
To unblock `serde_yaml` update for #8320

I wasn't sure what the policy was, so updated to the latest version of `insta` crate. Please, let me know if I should chose some other version instead.
near-bulldozer bot pushed a commit that referenced this issue Jan 19, 2023
To unblock serde_yaml update for #8320

I wasn't sure what the policy was, so updated to the latest version of `paperclip` crate. Please, let me know if I should chose some other version instead.

There is another very similar PR happening in parallel #8390 that is also necessary for the `serde_yaml` update and is blocking this PR, so feel free to take a look at that one as well ^^
@aborg-dev
Copy link
Contributor Author

The serde_yaml is update and TODOs in the code are fixed now, so closing this as done.

nikurt pushed a commit to nikurt/nearcore that referenced this issue Jan 30, 2023
To unblock `serde_yaml` update for near#8320

I wasn't sure what the policy was, so updated to the latest version of `insta` crate. Please, let me know if I should chose some other version instead.
nikurt pushed a commit to nikurt/nearcore that referenced this issue Jan 30, 2023
To unblock serde_yaml update for near#8320

I wasn't sure what the policy was, so updated to the latest version of `paperclip` crate. Please, let me know if I should chose some other version instead.

There is another very similar PR happening in parallel near#8390 that is also necessary for the `serde_yaml` update and is blocking this PR, so feel free to take a look at that one as well ^^
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-housekeeping Category: Refactoring, cleanups, code quality
Projects
None yet
Development

No branches or pull requests

1 participant