georust / gpx

Rust read/write support for GPS Exchange Format (GPX)
https://crates.io/crates/gpx
MIT License
102 stars 46 forks source link

Apply Clippy Suggestions #81

Closed 0b11001111 closed 1 year ago

0b11001111 commented 2 years ago

Hey folks, I had to burn some spare time and ran clippy -- -W clippy::pedantic against the project. I applied most of the suggestions and now running clippy -- -W clippy::pedantic -A clippy::used_underscore_binding passes.

The clippy::used_underscore_binding rule still fails because of Waypoint::_type. Renaming it would break the API, hence I did not do that. I suggest to rename the field in some future release to something that aligns with Rust's naming convention ;)

Additionally, I refactored the consume functions in the parser modules as some of the were super long. If applicable, I introduced a private try_from_attribute function that is used for component initialization.

Since this PR is mostly about cosmetics and doesn't change any semantics (hopefully), I did not put it in the changelog :P

0b11001111 commented 2 years ago

One could also think of adding the Clippy command to the github action in order to enforce consistent style for the project. Thoughts on that?

lnicola commented 2 years ago

Honestly, I'm not that big of a fan with the default set of warnings, not to mention pedantic. I don't think these bring much, and even the defaults sometimes have a lot of false positives, especially on larger projects. Blocking some new contributor's PR because clippy mistakenly believes that some collect is unnecessary is not worth it.

We can merge these ones, but I didn't have a chance to look over them yet.

lnicola commented 2 years ago

Regarding _type, we already have some breaking changes queued up for the next release, so we could take the opportunity to fix that.

0b11001111 commented 2 years ago

Honestly, I'm not that big of a fan with the default set of warnings, not to mention pedantic. I don't think these bring much, and even the defaults sometimes have a lot of false positives, especially on larger projects. Blocking some new contributor's PR because clippy mistakenly believes that some collect is unnecessary is not worth it.

Okay, I see. One counter argument is that, at least for me, it is confusing to see a lot of warnings on a fresh clone and it's easy to overlook which of them one added over the course of development. But that's just nitpicking and I feel like the project is overall in very good shape 🙃. Most issues of that kind can be caught in the review process anyway.

Regarding _type, we already have some breaking changes queued up for the next release, so we could take the opportunity to fix that.

What alternative do you suggest? Comming from Python, it's common to suffix identifiers that collide with key words with a _ or to intentionally misspell the identifier (e.g. class becomes klass). The latter one, however, triggers the spell checker of the IDEs so I'd go with type_

lnicola commented 2 years ago

One counter argument is that, at least for me, it is confusing to see a lot of warnings on a fresh clone and it's easy to overlook which of them one added over the course of development.

Please stop using -W pedantic and all will be well :-).

What alternative do you suggest?

type_, I suppose, though it isn't that much better :sweat_smile:.

0b11001111 commented 2 years ago

type_, I suppose, though it isn't that much better sweat_smile.

I agree but it makes the linter happy ¯\_(ツ)_/¯.

How about typ? It will be understood by English speakers and actually means "type" in German

lnicola commented 2 years ago

Nah, let's go with type_, it's pretty standard in an unrelated field, together with klass (but not Klasse :smile:).

0b11001111 commented 2 years ago

but not Klasse 😄

You'd wonder what people have tried 😄 http://www.fiber-space.de/EasyExtend/doc/teuton/teuton.htm

lnicola commented 2 years ago

@0b11001111 bist du noch da? :smile:

0b11001111 commented 2 years ago

Yes, just being busy these days :P I'll come back on this by next weekend, hopefully ;)

lnicola commented 1 year ago

Closing for inactivity. Please file another PR if you decide to come back to this, or maybe one for each lint.