ebarnard / rust-plist

A rusty plist parser.
MIT License
71 stars 42 forks source link

Some improvements #117

Closed amsam0 closed 1 year ago

amsam0 commented 1 year ago

Hi, this PR has a few improvements. If you want me to remove anything, please tell me!

// New usage (way easier!) let mut dictionary = Dictionary::new(); dictionary.insert("string", "test"); dictionary.insert("integer", 1);


If there's any other functions that would benefit from `impl Into`, I think it would be good to add it to those but `insert` was the main one I saw.
- Implement `From<Vec<u8>>` for `Value`

All tests were passing when I ran them locally, but it seems that this may not be backwards compatible.
amsam0 commented 1 year ago

Hey @ebarnard, sorry to bother you, do you have any thoughts on these changes? would it be possible to use impl Into for arguments, or should I make a fork with those changes since they don't seem to be backwards compatible?

amsam0 commented 1 year ago

I can revert the breaking changes and dependency update. May I have your permission to make a plist2 crate which would be a fork of this crate with those breaking changes?

ebarnard commented 1 year ago

I'm not sure there's any benefit to releasing either a major version or new crate for a very minor ergonomoic change, and a reasonable number of downsides to doing either. With that said, you don't need permission to fork open source software if you feel that it is worth doing.

amsam0 commented 1 year ago

Would you be willing to add this change as an optional feature? Maybe to the unstable feature?

ebarnard commented 1 year ago

What is your use case for this change? Do you have a lot of hand-written code where you are having to write .into()?

amsam0 commented 1 year ago

https://github.com/SideStore/minimuxer/blob/master/src/muxer.rs#L178-L201

yes, and I expect we will have to do a lot more of this if/when we reimplement some C libraries we depend on in Rust.

ebarnard commented 1 year ago

If it's just for one project, would it not be simpler to define your own extension trait, e.g.

pub trait DictionaryExt {
    fn add(&mut self, k: impl Into<String>, v: impl Into<Value>) -> Option<Value>;
}

impl DictionaryExt for Dictionary { ... }
amsam0 commented 1 year ago

It would be for multiple projects, and I'd rather not copy that trait to every project. (although, I could probably make a crate for it)

I've reverted the dependency update, breaking change and now the From implementation uses Data instead of Vec<u8>. Let me know if there's anything else you want me to change.

amsam0 commented 1 year ago

Removing extern crate may drop support for older compilers

amsam0 commented 1 year ago

I might separate this PR into smaller ones in the future, but other than implementing From for Value it's pretty useless

DictionaryExt trait might be useful in the future, with things like From<[(K, V)]> and append(). It would also be good to make a ToBytes trait or something similar as a helper method and from_value similar to serde_json.