Apply Clippy Suggestions #81

0b11001111 · 2022-08-13T13:47:05Z

[ x] I agree to follow the project's code of conduct.
[ ] I added an entry to CHANGELOG.md if knowledge of this change could be valuable to users.

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

Satisfies: ```shell cargo clippy -- \ -W clippy::pedantic \ -A clippy::used_underscore_binding \ -A clippy::missing_errors_doc \ -A clippy::too_many_lines ```

0b11001111 · 2022-08-13T13:49:04Z

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?

Satisfies: ```shell cargo clippy -- \ -W clippy::pedantic \ -A clippy::used_underscore_binding \ -A clippy::missing_errors_doc ``` Introduce a pattern where alongside `consume` a new function `try_from_attributes` is implemented for components (`Gpx` and `Waypoint` so far). This shortens the usually very long `consume` and gives the reader a clear distinction about the two parsing phases.

Satisfies: ```shell cargo clippy -- \ -W clippy::pedantic \ -A clippy::used_underscore_binding ```

lnicola · 2022-08-13T14:20:46Z

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 · 2022-08-13T14:24:13Z

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

0b11001111 · 2022-08-13T14:49:48Z

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 · 2022-08-13T14:52:46Z

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 😅.

…and `Link` Satisfies: ```shell cargo clippy -- \ -W clippy::pedantic ```

0b11001111 · 2022-08-13T15:00:07Z

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 · 2022-08-13T15:07:24Z

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

0b11001111 · 2022-08-13T15:16:23Z

but not Klasse 😄

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

lnicola · 2022-08-13T19:47:04Z

src/writer.rs

-        GpxVersion::Gpx10 => Ok("http://www.topografix.com/GPX/1/0"),
-        GpxVersion::Gpx11 => Ok("http://www.topografix.com/GPX/1/1"),
-        version => Err(GpxError::UnknownVersionError(version)),
+        GpxVersion::Gpx10 => Ok("https://www.topografix.com/GPX/1/0"),


Let's not do this, that URL identifies the schema, and it's not meant to be browsed. See the targetNamespace in https://www.topografix.com/GPX/1/1/gpx.xsd.

lnicola · 2022-08-13T19:52:23Z

src/parser/gpx.rs

-            } else {
-                break;
-            }
+        let next_event = match context.reader.peek() {


I like this (nested matching in next_event), but the try_from_attributes just looks like a not very successful attempt to shrink the function.

Not strongly against it, but it feels a bit pointless.

lnicola · 2022-08-13T19:53:28Z

src/parser/waypoint.rs

-            } else {
-                break;
-            }
+        let next_event = match context.reader.peek() {


Same as in the other file.

lnicola · 2022-08-13T19:53:33Z

src/parser/waypoint.rs

@@ -44,23 +43,24 @@ pub fn consume<R: Read>(context: &mut Context<R>, tagname: &'static str) -> GpxR
    if !(-180.0..180.0).contains(&longitude) {
        return Err(GpxError::LonLatOutOfBoundsError(
            "Longitude",
-            "[-180.0, 180.0",
+            "[-180.0, 180.0)",


lnicola · 2022-08-13T20:06:13Z

src/reader.rs

+///
+/// # Errors
+///
+/// Propagates errors from [consume](crate::parser::gpx::consume).


I'd drop these or phrase them differently, consume isn't even a public function.

lnicola · 2022-08-13T20:09:25Z

CHANGELOG.md

@@ -6,6 +6,9 @@
                                                 fix error handling bug on track parsing,
                                                 apply Clippy suggestion
 - [#80](https://github.com/georust/gpx/pull/72): Update MSRV to 1.57.0
+- [#81](https://github.com/georust/gpx/pull/81): Rename the field `_type` to `type_` for `Route`, `Track`, `Waypoint`


I'd simply say

Rename Route::_type, Track::_type, Waypoint::_type, Link::_type to type_.

The reasoning can be found in the PR anyway.

lnicola · 2022-08-21T19:06:55Z

@0b11001111 bist du noch da? 😄

0b11001111 · 2022-08-22T09:21:00Z

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

lnicola · 2023-05-30T08:45:23Z

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

0b11001111 added 2 commits August 13, 2022 13:13

Add IntelliJ and VSCode config folders to gitignore

922dad5

Apply most suggestions by Clippy in pedantic mode

854b03c

Satisfies: ```shell cargo clippy -- \ -W clippy::pedantic \ -A clippy::used_underscore_binding \ -A clippy::missing_errors_doc \ -A clippy::too_many_lines ```

0b11001111 added 3 commits August 13, 2022 15:50

Refactor consume functions for remaining parser modules, see 7b28155

c382a16

Apply more suggestions by Clippy in pedantic mode

26db030

Satisfies: ```shell cargo clippy -- \ -W clippy::pedantic \ -A clippy::used_underscore_binding ```

0b11001111 force-pushed the clippy-suggestions branch from 7cbf03c to 26db030 Compare August 13, 2022 13:51

0b11001111 changed the title ~~Clippy suggestions~~ Apply Clippy Suggestions Aug 13, 2022

Rename the field _type to type_ for Route, Track, Waypoint …

6a9b3a3

…and `Link` Satisfies: ```shell cargo clippy -- \ -W clippy::pedantic ```

lnicola reviewed Aug 13, 2022

View reviewed changes

src/parser/waypoint.rs

} else {

break;

}

let next_event = match context.reader.peek() {

Copy link

Member

lnicola Aug 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as in the other file.

lnicola reviewed Aug 13, 2022

View reviewed changes

lnicola closed this May 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply Clippy Suggestions #81

Apply Clippy Suggestions #81

0b11001111 commented Aug 13, 2022

0b11001111 commented Aug 13, 2022

lnicola commented Aug 13, 2022 •

edited

lnicola commented Aug 13, 2022

0b11001111 commented Aug 13, 2022 •

edited

lnicola commented Aug 13, 2022

0b11001111 commented Aug 13, 2022 •

edited

lnicola commented Aug 13, 2022 •

edited

0b11001111 commented Aug 13, 2022 •

edited

lnicola Aug 13, 2022

lnicola Aug 13, 2022

lnicola Aug 13, 2022

lnicola Aug 13, 2022

lnicola Aug 13, 2022

lnicola Aug 13, 2022

lnicola commented Aug 21, 2022

0b11001111 commented Aug 22, 2022

lnicola commented May 30, 2023

Apply Clippy Suggestions #81

Apply Clippy Suggestions #81

Conversation

0b11001111 commented Aug 13, 2022

0b11001111 commented Aug 13, 2022

lnicola commented Aug 13, 2022 • edited

lnicola commented Aug 13, 2022

0b11001111 commented Aug 13, 2022 • edited

lnicola commented Aug 13, 2022

0b11001111 commented Aug 13, 2022 • edited

lnicola commented Aug 13, 2022 • edited

0b11001111 commented Aug 13, 2022 • edited

lnicola Aug 13, 2022

Choose a reason for hiding this comment

lnicola Aug 13, 2022

Choose a reason for hiding this comment

lnicola Aug 13, 2022

Choose a reason for hiding this comment

lnicola Aug 13, 2022

Choose a reason for hiding this comment

lnicola Aug 13, 2022

Choose a reason for hiding this comment

lnicola Aug 13, 2022

Choose a reason for hiding this comment

lnicola commented Aug 21, 2022

0b11001111 commented Aug 22, 2022

lnicola commented May 30, 2023

lnicola commented Aug 13, 2022 •

edited

0b11001111 commented Aug 13, 2022 •

edited

0b11001111 commented Aug 13, 2022 •

edited

lnicola commented Aug 13, 2022 •

edited

0b11001111 commented Aug 13, 2022 •

edited