search

klingtnet / rosc

An OSC library for Rust.
Apache License 2.0
173 stars 25 forks source link

Why are the arguments an option? #7

Closed piegamesde closed 4 years ago

piegamesde commented 4 years ago

See https://docs.rs/rosc/0.2.0/rosc/struct.OscMessage.html#structfield.args . They are of type Option<Vec<OscType>>. But what is the difference between None and Some(vec![])? If None has no additional meaning to "no arguments specified", I'd prever having a simple Vec<OscType>.

klingtnet commented 4 years ago

My intention was to indicate that it is optional for an OscMessage to have arguments. I don' think that this is an issue since one can use if let ... or similar constructs to easily access the arguments if any. Also, removing the Option from the args type would break existing implementations. I will keep the issue open for now, maybe there are good arguments for removing Option from args.

piegamesde commented 4 years ago

But what is the advantage of giving None instead of an empty vector if there are no arguments? This forces users to always do an empty check, which is redundant in many cases (e.g. iterating over all arguments). IMO, an empty vector and no vector should have different meaning, because I don't like having two ways to encode the same value. If both representations have the same meaning, the Option is redundant and can be removed.

Also, removing the Option from the args type would break existing implementations.

The major version number is 0, so if this crate follows semantic versioning this should be expected. Maybe we can keep the old implementation using compile features or so for a while if this really is a concern.

https://www.reddit.com/r/rust/comments/9neuby/empty_vec_or_optionvec_none_and_why/

klingtnet commented 4 years ago

I still see not enough value in this change to deliberately break existing implementations. You are free to fork this library.

klingtnet commented 4 years ago

Lets do a vote, :rocket: for removing the Option and :tada: for keeping it.

mitchmindtree commented 4 years ago

This did always strike me as odd, thanks for posting this @piegamesde. I wouldn't mind the breakage, as long as it's in a 0.3 and not a 0.2.x so it doesn't break downstream dependencies, otherwise :rocket: :dancer:

klingtnet commented 4 years ago

@mitchmindtree Thank you for you response.

I will keep the issue open until the end of the week and then decide based on the votes.

klingtnet commented 4 years ago

Looks like we have a clear winner here, and I've got to admit that I see that Option is redundant. I will remove it and and close this issue then.

klingtnet commented 4 years ago

@piegamesde Thank you for discussing this issue!