Skip to content

chore: migrate from deprecated serde_yaml to serde_yml#9

Closed
nhomble wants to merge 1 commit intomainfrom
chore/migrate-serde-yml
Closed

chore: migrate from deprecated serde_yaml to serde_yml#9
nhomble wants to merge 1 commit intomainfrom
chore/migrate-serde-yml

Conversation

@nhomble
Copy link
Owner

@nhomble nhomble commented Dec 30, 2025

serde_yaml was deprecated in March 2024. serde_yml is a maintained fork with API-compatible interface.

serde_yaml was deprecated in March 2024. serde_yml is a maintained
fork with API-compatible interface.
@claude
Copy link

claude bot commented Dec 30, 2025

PR Review: Migrate from deprecated serde_yaml to serde_yml

Summary

This PR addresses the deprecation of serde_yaml by migrating to the maintained fork serde_yml. The changes are minimal and focused, affecting only the dependency declaration and two function implementations.


Code Quality & Best Practices ✅

Strengths:

  • Minimal, focused changes: Only touches what's necessary (2 files, 3 additions, 3 deletions)
  • API compatibility: serde_yml is API-compatible with serde_yaml, making this a drop-in replacement
  • Consistent naming: Both yaml_to_json and json_to_yaml functions updated consistently in src/query.rs:48 and src/query.rs:52
  • Clear commit message: Follows conventional commits format with appropriate context

Potential Issues ⚠️

Critical - Version Specification:
The Cargo.toml specifies serde_yml = "0.0" which is concerning:

  • Version 0.0.x indicates the crate is in very early development
  • Using "0.0" will match any 0.0.x version, potentially pulling in breaking changes
  • Recommendation: Specify an exact version or use a more precise constraint like "0.0.12" (check latest stable on crates.io)

Cargo.lock consideration:

  • The PR diff doesn't show Cargo.lock changes
  • Ensure Cargo.lock is committed if this is an application (which it appears to be based on the CLI nature)
  • This ensures reproducible builds and version consistency

Testing Coverage ✅

Existing tests should validate the migration:
The codebase has comprehensive test coverage:

  • Unit tests in src/query.rs:86-126 covering yaml_to_json, json_to_yaml, and query/mutation functionality
  • Integration tests in tests/integration.rs with fixture-based testing
  • The existing tests will catch any API incompatibilities

Recommendation:

  • Run the full test suite (cargo test) to verify no regressions
  • Consider running with the old dependency first to establish a baseline, then with the new one

Performance Considerations ✅

No expected performance impact:

  • This is a direct dependency swap with API compatibility
  • Both crates use similar YAML parsing approaches
  • Performance characteristics should be equivalent

Security Considerations ⚠️

Dependency maintenance:

  • ✅ Moving from deprecated to maintained fork is good for security
  • ⚠️ serde_yml being at version 0.0.x suggests it's very new - verify:
    • Crate ownership and maintainer reputation
    • Community adoption and usage
    • Active maintenance and security response

Recommendation:

  • Verify serde_yml is the official/recommended fork of serde_yaml
  • Check if there's a more stable alternative or if serde_yaml has been un-deprecated
  • Monitor the dependency for security advisories

Action Items

Before merging:

  1. Update version constraint in Cargo.toml to a specific version (e.g., "0.0.12" instead of "0.0")
  2. Run tests: Execute cargo test to ensure all tests pass
  3. Verify build: Run cargo build --release to check for any compilation issues
  4. Check Cargo.lock: Ensure it's updated and committed
  5. Validate the fork: Confirm serde_yml is the recommended replacement for serde_yaml

Verdict

The migration approach is sound, but the version specification needs attention before merging. Once the version is pinned appropriately and tests pass, this should be safe to merge.

Suggested change for Cargo.toml:

-serde_yml = "0.0"
+serde_yml = "0.0.12"  # or whatever the latest stable version is

@nhomble nhomble closed this Dec 30, 2025
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.

1 participant