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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
d42d187
cargo clippy
mastrzyz Sep 13, 2022
0200111
updated.
mastrzyz Sep 13, 2022
ab3c6bf
yaml
mastrzyz Sep 13, 2022
a9b9a38
proper component.
mastrzyz Sep 13, 2022
319deba
up.
mastrzyz Sep 13, 2022
7075993
style.
mastrzyz Sep 13, 2022
6133d3e
dis-allow
mastrzyz Sep 13, 2022
1a2e214
updated.
mastrzyz Sep 13, 2022
76ebfa4
updated.
mastrzyz Sep 13, 2022
4381359
fix.
mastrzyz Sep 22, 2022
990137e
Merge branch 'hyperium:master' into master
mastrzyz Sep 22, 2022
7b89b31
fixed format.
mastrzyz Sep 22, 2022
7ff325b
up.
mastrzyz Sep 23, 2022
d9ff2b7
Updated.
mastrzyz Sep 23, 2022
4fd905d
Merge branch 'hyperium:master' into master
mastrzyz Sep 23, 2022
edd0f53
Deny!
mastrzyz Oct 4, 2022
4ae5fcc
Merge branch 'master' of https://github.com/mastrzyz/hyperclip
mastrzyz Oct 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions .github/workflows/CI.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ jobs:
runs-on: ubuntu-latest
needs:
- style
- clippy
- test
- msrv
- miri
Expand Down Expand Up @@ -45,6 +46,26 @@ jobs:
command: fmt
args: --all -- --check

clippy:
name: Static Code Analysis ( Clippy )
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v1

- name: Install Rust
uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: stable
override: true
components: clippy

- name: Clippy Check
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 :/

test:
name: Test ${{ matrix.rust }} on ${{ matrix.os }}
needs: [style]
Expand Down
10 changes: 10 additions & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,20 @@
#![deny(missing_docs)]
#![deny(missing_debug_implementations)]
#![deny(clippy::self_named_module_files)]
#![cfg_attr(test, deny(rust_2018_idioms))]
#![cfg_attr(all(test, feature = "full"), deny(unreachable_pub))]
#![cfg_attr(all(test, feature = "full"), deny(warnings))]
#![cfg_attr(all(test, feature = "nightly"), feature(test))]
#![cfg_attr(docsrs, feature(doc_cfg))]
#![allow(
clippy::needless_borrow,
clippy::module_inception,
clippy::useless_format,
clippy::redundant_clone,
clippy::manual_non_exhaustive,
dead_code,
mastrzyz marked this conversation as resolved.
Show resolved Hide resolved
unused_imports
mastrzyz marked this conversation as resolved.
Show resolved Hide resolved
)]

//! # hyper
//!
Expand Down