search

RustAudio / ogg

Ogg container decoder and encoder written in pure Rust
Other
113 stars 21 forks source link

2021 edition, tokio 1.0 #26

Closed yotamofek closed 2 years ago

yotamofek commented 2 years ago

Hey :)

I saw @est31 mention here that there were plans to release a new version updated for the 2021 edition once that is stable, and to upgrade to latest tokio dep. Thought I'd contribute those two things at one go.

This PR is best reviewed as separate commits. I made a decision that I'm not sure the maintainers will agree with to run cargo fmt and then fix all cargo clippy warnings/lints (including removing some #![allow(...)] statements. Coding style is a completely subjective matter, but I think that adhering to the "community standards" (as dictated by the rustfmt and clippy tools) allows more people to be able to easily contribute to the project. I don't mind undoing those changes and reapplying the other commits on top :)

est31 commented 2 years ago

I made a decision that I'm not sure the maintainers will agree with to run cargo fmt and then fix all cargo clippy warnings/lints (including removing some #![allow(...)] statements

Yeah cargo fmt doesn't support the code style of this project atm. It's also a bit disrespectful IMO to just make a cold PR like that. I'm not forcing my code style onto other people either when I make PRs. If you check the history there have been similar PRs like this one. If I wanted cargo fmt, I'd run it myself.

The clippy warnings have been disabled for a reason, most were suggesting code to be made worse or were stylistically bad choices. As to the constification work, edition 2021, tokio update, etc. it's all good. I don't like the ? operator though as it sabotages debugging compared to the macro based approach.

est31 commented 2 years ago

I've done the 2021 edition update and the const fn adoption myself. Feel free to file a new PR that updates to tokio 1.0. It's a bit more involved and your help would be much appreciated :).

yotamofek commented 2 years ago

I definitely didn't mean to be disrespectful. Which is why I gave a rationale basis for my decision and suggested undoing those changes. I'm sorry you felt disrespected.

Will open another PR.

(And IMHO, calling clippy lints "stylistically bad" is disrespectful because it assumes your subjective opinion of style is more correct than others'.)

est31 commented 2 years ago

I'm sorry you felt disrespected.

Thanks for the apology.

And IMHO, calling clippy lints "stylistically bad" is disrespectful because it assumes your subjective opinion of style is more correct than others'

Sorry for that. I was a bit too harsh in my earlier comment I think, and regret that now. I don't like some of clippy's suggestions, some are really good. And yeah it's a matter of taste. Also, you may fix the default clippy lint by adding default impls as long as you leave around new. Looking forward to your PR!