Skip to content

make json_macros compatible with serde 0.9.x #33

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yannleretaille
Copy link

  • serde_json 0.9.x uses serde_json::Map instead of BTreeMap
  • added tests for (pretty)printing
  • passes all tests for rustc-serialize and serde_json

Now that serde introduced its own json! macro, you might consider dropping support for serde in the next version.

@dtolnay
Copy link

dtolnay commented Feb 27, 2017

I would agree with dropping Serde support. I filed #34 to follow up.

@yannleretaille do you have any reasons for using this library with serde 0.9? What motivates the PR?

@yannleretaille
Copy link
Author

yannleretaille commented Feb 27, 2017

@dtolnay Actually, I just didn't notice serde_json had introduced a macro until it was already ported :D There might be others who might appreciate that stuff just continues to work as it did before.

@dtolnay
Copy link

dtolnay commented Feb 27, 2017

Cool. I think the most valuable course of action would be to drop Serde support in this library in favor of serde_json's json! macro (#34) and use the logic from serde_json's json! macro to provide a stable version of this library that works with rustc-serialize (#35).

Although note that rustc-serialize is slated to be deprecated in Rust 1.18... see here.

@yannleretaille
Copy link
Author

Sounds like a good plan! #35 looks fairly straightforward to implement and would make a lot of sense. For #34 I think it would be best to keep the feature flag and to either:

  • throw an error with instructions on how to move to serde_json::json
  • or throw a warning and use serde_jsons version

Farewell, json_macros, you served us well!

@dtolnay
Copy link

dtolnay commented Feb 27, 2017

If it were my library I wouldn't bother with either of those. Just put a prominent note in the readme and people will find it eventually.

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.

2 participants