search

9-FS / nhentai_archivist

downloads hentai from nhentai.net and converts to CBZ
MIT License
115 stars 7 forks source link

fix: clippy lint warnings #4

Closed TristanWasTaken closed 2 months ago

TristanWasTaken commented 2 months ago

I ran the default clippy lints on the project. Clippy warnings ensure that you write idiomatic Rust (I recommend reading the Rust book if you haven't already), such as not calling needless returns.

At the very least the commit f953f757843205d1752f550dea0a06ccb23e2e4a should be merged, because clippy::suspicious_open_options might actually cause issues.

If the file already exists, it will be overwritten when written to, but the file will not be truncated by default. If less data is written to the file than it already contains, the remainder of the file will remain unchanged, and the end of the file will contain old data.

Changes:

9-FS commented 2 months ago

Hi, thank you for contributing. This is actually the first time someone has ever created a merge request on one of my projects. I like most of your commits, but not all of them. Is there a way to go about that?

TristanWasTaken commented 2 months ago

Hi, thank you for contributing. This is actually the first time someone has ever created a merge request on one of my projects. I like most of your commits, but not all of them. Is there a way to go about that?

  1. You can create a new commit with the things you want to change.
  2. You can revert (so) the commit if you want to remove the changes of the whole commit.
  3. You can review the pr and request changes from me. <-- I prefer this, because I'm working on another pr that applies rustfmt
9-FS commented 2 months ago

Thanks! I'll give you a detailled list of changes later.

Please don't bother with the rustfmt, I have tried it and I think it is not ready yet. It just destroys everything.

TristanWasTaken commented 2 months ago

Thanks!

Please don't bother with the rustfmt, I have tried it and I think it is not ready yet. It just destroys everything.

It just really doesn't like where you put your comments (very likely because it's not idiomatic Rust that is described in the Rust book). It works really well when I move the comments to new lines. For example when I moved the comment out of the function declaration:

- impl From<Hentai> for ComicInfoXml
- {
-    fn from(hentai: Hentai) -> Self // Hentai -> ComicInfo
-    {
+ impl From<Hentai> for ComicInfoXml {
+    // Hentai -> ComicInfo
+    fn from(hentai: Hentai) -> Self {
9-FS commented 2 months ago

fix clippy::needless_return lint warnings (b4cd764)

denied

fix clippy::needless_late_init lint warnings (c907558)

denied

fix clippy::map_flatten lint warnings (12433f5, 089fc44)

approved

fix clippy::to_string_in_format_args lint warnings (36e184a)

approved

fix clippy::expect_fun_call lint warnings (d39b73c)

approved

fix clippy::ptr_arg lint warnings (281edc8)

approved

fix clippy::trim_split_whitespace lint warning (12805d0)

approved

fix clippy::redundant_pattern_matching lint warning (6cdc5b0)

approved

fix clippy::suspicious_open_options lint warning (f953f75)

approved, big thanks for that

fix clippy::needless_borrow lint warnings (2d01a0b)

approved

TristanWasTaken commented 2 months ago

fix clippy::needless_return lint warnings (b4cd764)

denied

fix clippy::needless_late_init lint warnings (c907558)

denied

so that is what you meant with "It just destroys everything" 💡 Judging from your code style, I'm guessing you come from a C# background? I'm sorry, but the Rust formatter will never be "ready" since it's just not how Rust code is usually written 😅

You should document in the README.md or a CONTRIBUTING.md file that your code style is different than the community-wide agreed upon standard (idiomatic Rust). The Clippy lints and especially rustfmt follow that agreed upon style guide.

“idiomatic” language is a usage which follows the style and practices (the idioms) of the wider community.

If you don't document it, there will be clashes with other Rust devs that want to contribute and follow the normal Rust code style (e.g. C# code style for Rust just looks weird for me).

TristanWasTaken commented 2 months ago

With the last 4 commits, I

9-FS commented 2 months ago

I personally used rustfmt with the following settings:

blank_lines_upper_bound      = 2
brace_style                  = "AlwaysNextLine"
combine_control_expr         = false
control_brace_style          = "AlwaysNextLine"
enum_discrim_align_threshold = 50
hex_literal_case             = "Upper"
indent_style                 = "Block"
max_width                    = 200
struct_field_align_threshold = 50

But it kept moving comments into next lines, moving curly braces to areas where there were not supposed to be, clear bugs in edge cases, and so on. I ultimately gave up for now and will come back to it at a later time.

My needlessly early declarations and needless explicit returns are because I like structuring my functions in 3 parts, in all programming languages:

  1. declaration: Contains variable declarations and comments about what the variables do and serves as quick lookup. This is why I do so many unnecessary early declarations. I don't do early declarations with temporary/helper variables.
  2. initialisation, preparation, checking for invalid values: Contains preparing code and often contains early returns out of the function if conditions aren't met, which prevents nesting. Especially because I have this style of multiple return points in a function, I strongly prefer explicit return statements everywhere. I only don't use return in closures, because I don't "leave" the actual function.
  3. actual work the function is supposed to do