search

ComunidadAylas / vorbis-rs

🔊 Rust bindings for the best-in-breed C libraries of the Vorbis audio codec and Ogg container encapsulation.
https://crates.io/crates/vorbis_rs
BSD 3-Clause "New" or "Revised" License
16 stars 4 forks source link

More readable generics on VorbisEncoder::new() #9

Closed marceline-cramer closed 1 year ago

marceline-cramer commented 1 year ago

This is totally up to taste but I find this style of writing generics here to be more readable than the hybrid impl IntoIterator<...> combined with the angle bracket bounds syntax.

AlexTMjugador commented 1 year ago

Hey, thank you for the pull request! :heart:

Incidentally, I find the opposite of what you say to be true: I consider it more readable to use impl for generic parameters in function arguments whenever possible. Rust's RFC 1951 describes some reasons why this syntax may be considered more ergonomic and consistent than generic parameters.

I've seen quite a bit of Rust code in the wild not use impl for this purpose, but I'm not sure why that's the case. That RFC made it into stable Rust years ago, but perhaps people just kept using and teaching the syntax they already knew without thinking about it. Or maybe most Rust programmers developed a taste for not using it in this situation, but I didn't.

Since this is purely a stylistic choice, I think the right decision would be to use the style most people are comfortable with, and in case of doubt or conflict, whatever makes the project maintainers happier. Do you have any data about what's the most common better regarded choice?

marceline-cramer commented 1 year ago

I generally also agree with the use of impl there, but in my opinion it adds a lot of clutter to this already-crowded function definition when it's combined with the additional trait bounds in the type parameters. Personally, seeing generic trait bounds both using impl and type parameter syntax threw me for a loop when reading this function in the documentation.

If you're not content with my preference in style, feel free to close this PR. I take no offense to it; this change was just a quick suggestion that I whipped up in like thirty seconds. I don't have any second opinions or similar cases on hand to back up my preference, but maybe something to consider would be switching this function to use the Builder pattern instead, which is most definitely commonly used in Rust for complex initialization code like this. I'm not sure how that would solve the problem at hand with complex generic definitions, but it would at least decouple the generics from the rest of the extraneous information on this function definition.

AlexTMjugador commented 1 year ago

Indeed, I agree with your point that the new function is on the verge of having too many arguments. I think that using the builder pattern instead would be a great way to reduce that complexity and avoid cluttering a single method with a lot of generics :heart:

Would you like to reconsider your PR to introduce a VorbisEncoderBuilder instead? I'd gladly merge that!

marceline-cramer commented 1 year ago

Sure! I can give that a shot, and I'll request a review when I have a draft ready. Thank you for the informed discussion. :)

AlexTMjugador commented 1 year ago

Hey @marceline-cramer, just a friendly ping. Do you have any plans to share that draft for review? :smile:

marceline-cramer commented 1 year ago

This last month has been the busiest of my life and I haven't been able to spend time on open source, but things are finally clearing up and I can work on the encoder builder this coming weekend.

AlexTMjugador commented 1 year ago

It's great to hear that you can finally get your hands on it! :heart:

Please stay in touch and take the time you need. Thank you again for the PR.

marceline-cramer commented 1 year ago

I'm really sorry but I don't think I'll be able to do this any time in the foreseeable future, so I need to drop this. Best wishes to you, I sincerely hope that my input helps!

AlexTMjugador commented 1 year ago

Your input was very useful, thanks!

In general, I saw no issue with keeping this PR open. I didn't have plans for the next release, so nothing became "stale" or "blocked" due to it. However, I totally understand your decision. Please feel free to get in touch again :smile:

AlexTMjugador commented 1 year ago

I recalled this PR was a thing and had some time to dedicate to its idea, so I introduced a VorbisEncoderBuilder in commit 1d19c62.

Feel free to let me know if you are interested in getting a quick release with that (otherwise I'll probably wait a bit in case something that needs fixing shows up), or if you have any feedback on it!

Edit: as an additional goodie, I figured out the builder would be a good place to introduce optional Ogg stream serial generation logic, as getting it 100% right is a bit trickier than it might seem at a first glance.