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

Clippy Analysis to CI #2979

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Clippy Analysis to CI #2979

wants to merge 17 commits into from

Conversation

mastrzyz
Copy link

@mastrzyz mastrzyz commented Sep 13, 2022

Enables Clippy in the CI as a need of the top-level ci-pass job.
Resolution for #2977
Disabled the current violations :

#![allow(
    clippy::needless_borrow,
    clippy::module_inception,
    clippy::useless_format,
    clippy::redundant_clone,
    clippy::manual_non_exhaustive,
    dead_code,
    unused_imports
)]

And explicitly enabled the one requested one :

#![deny(clippy::self_named_module_files)]

Result :

    Checking hyper v1.0.0-dev.0 (/home/runner/work/hyperclip/hyperclip)
    Finished dev [unoptimized + debuginfo] target(s) in 53.42s

If we have a violation , we would see :

error: could not compile `hyper` due to previous error

@mastrzyz mastrzyz marked this pull request as ready for review September 13, 2022 05:40
@mastrzyz mastrzyz marked this pull request as draft September 13, 2022 05:41
src/lib.rs Outdated Show resolved Hide resolved
@mastrzyz mastrzyz marked this pull request as ready for review September 13, 2022 06:02
@mastrzyz mastrzyz changed the title cargo clippy Clippy Analysis to CI Sep 13, 2022
uses: actions-rs/cargo@v1
with:
command: clippy

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking we could add an args: -- -Aclippy::all -Wclippy::specific_lint_name, and thus not need to change the actual source code (like the fmt job above).

Copy link
Author

Choose a reason for hiding this comment

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

Right that's an option but might not be 100% intuitive to someone why their CI Build is failing when locally it isn't?

Copy link
Author

Choose a reason for hiding this comment

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

@seanmonstar ^^ My concern is something will fail in CI and not locally .

Copy link
Author

Choose a reason for hiding this comment

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

I have reverted the change to the main lib file , verified the build is green ! -> https://github.com/mastrzyz/hyperclip/actions/runs/3110424028/jobs/5041594746

Copy link
Member

Choose a reason for hiding this comment

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

My concern is something will fail in CI and not locally .

I agree that changing the top level module would prevent this, but since we're allowing everything except self_named_module_files, this should be fine.

Although now that I think about it, changing the top level module might not be a bad idea at all 😄

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I wonder, does this actually need to be -D and not -W? I suppose just a warning won't stop the CI job from exiting successfully. (I believe there's one instance in the repo that should be failing this lint, hence why I wrote the issue, but seems like the job is happy.)

Copy link
Author

Choose a reason for hiding this comment

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

image

yeah it's warning but the step is still green here.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we make it -D? We want the CI job to fail, not just yell and then get forgotten.

(I would expect this to trigger a complaint about the ext.rs module.)

Copy link
Author

Choose a reason for hiding this comment

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

Sorry I misread -> Updated.

I verified that -D does in fact "Disallow"

image

But for the clippy rule we decided on enabling, there isn't any violation reported in ext.rs :/

@mastrzyz mastrzyz requested review from seanmonstar and dswij and removed request for seanmonstar and dswij September 22, 2022 05:26
src/lib.rs Outdated
Comment on lines 9 to 15
#![allow(
clippy::needless_borrow,
clippy::module_inception,
clippy::useless_format,
clippy::redundant_clone,
clippy::manual_non_exhaustive
)]
Copy link
Member

Choose a reason for hiding this comment

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

We can avoid adding these lines directly into src/lib.rs with an argument when we call clippy in CI, as in seanmonstar's comment

Copy link
Author

@mastrzyz mastrzyz left a comment

Choose a reason for hiding this comment

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

Done!

src/lib.rs Outdated Show resolved Hide resolved
uses: actions-rs/cargo@v1
with:
command: clippy

Copy link
Author

Choose a reason for hiding this comment

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

Right that's an option but might not be 100% intuitive to someone why their CI Build is failing when locally it isn't?

uses: actions-rs/cargo@v1
with:
command: clippy

Copy link
Author

Choose a reason for hiding this comment

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

@seanmonstar ^^ My concern is something will fail in CI and not locally .

src/lib.rs Outdated Show resolved Hide resolved
uses: actions-rs/cargo@v1
with:
command: clippy

Copy link
Author

Choose a reason for hiding this comment

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

I have reverted the change to the main lib file , verified the build is green ! -> https://github.com/mastrzyz/hyperclip/actions/runs/3110424028/jobs/5041594746

@mastrzyz mastrzyz requested review from dswij and removed request for seanmonstar September 23, 2022 04:46
@mastrzyz mastrzyz requested review from seanmonstar and removed request for dswij October 4, 2022 04:47
@palango palango mentioned this pull request Feb 28, 2023
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.

3 participants