Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup (clippy warnings) #47

Merged
merged 5 commits into from
Nov 22, 2021
Merged

Cleanup (clippy warnings) #47

merged 5 commits into from
Nov 22, 2021

Conversation

hoijui
Copy link
Contributor

@hoijui hoijui commented Nov 22, 2021

Almost all of these changes were suggested by:

cargo clippy --release -- --deny clippy::pedantic

Exceptions:

  • Moves expression.rs to expression/mod.rs
  • Moves text.rs to text/mod.rs

@Jake-Shadle
Copy link
Member

Please don't rename the files to mod.rs, mod.rs makes the code organization more unclear and less easy to grep through (though it's not a problem with this repo).

@Jake-Shadle
Copy link
Member

While I like the idea of fixing these lints, the problem is that we will never run with pedantic in a CI situation since we will only ever check for the lints specified in the lib.rs file, but I suppose this is fine as a one time change.

@hoijui
Copy link
Contributor Author

hoijui commented Nov 22, 2021

While I like the idea of fixing these lints, the problem is that we will never run with pedantic in a CI situation since we will only ever check for the lints specified in the lib.rs file, but I suppose this is fine as a one time change.

hmmm.. I noticed that pedantic takes quite some more time to run then your clippy settings.. is it because of this?
or rather because you don't want to scare-off contributors, or.. something else? (I am still quite new to rust)

@hoijui
Copy link
Contributor Author

hoijui commented Nov 22, 2021

I undid the renames to mod.rs, and added aad225e
.. though that file (src/text.rs) looks like it might be generated; is it?

@Jake-Shadle
Copy link
Member

hmmm.. I noticed that pedantic takes quite some more time to run then your clippy settings.. is it because of this?
or rather because you don't want to scare-off contributors, or.. something else? (I am still quite new to rust)

We have our own set of lints that are informed by our practical experience in real codebases. We do use specific lints from the pedantic set because we find them useful, but generally they can be noise, at least in larger codebases.

.. though that file (src/text.rs) looks like it might be generated; is it?

Yes, it is generated from the update program.

Copy link
Member

@Jake-Shadle Jake-Shadle left a comment

Choose a reason for hiding this comment

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

Thanks!

@mergify mergify bot merged commit 7cf62f8 into EmbarkStudios:main Nov 22, 2021
@hoijui
Copy link
Contributor Author

hoijui commented Nov 22, 2021

thank you! :-)
... lots to learn here.. will maybe use your lints too for my projects. :-)
It seems to me, that your update program works similarly like build.rs. You do it this way because it is more... flexible/powerful somehow?

@Jake-Shadle
Copy link
Member

Build.rs would be run for all builds that depend on this project, which (in this case) is also doing network calls, which are something that should never be done during a build. The SPDX list is only updated a few times a year, so it's vastly easier to just update it manually.

@hoijui hoijui deleted the cleanup branch November 23, 2021 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants