Skip to content

Conversation

@tanguysegarra
Copy link
Collaborator

Closes #290

@Skallwar
Copy link
Member

Skallwar commented Sep 2, 2022

I think we should use something like this https://github.com/marketplace/actions/rust-clippy-check

@tanguysegarra
Copy link
Collaborator Author

I believe we can trust the action you linked @Skallwar, I'll implement the change later on :) thanks for the tip

@tanguysegarra
Copy link
Collaborator Author

tanguysegarra commented Sep 11, 2022

@Skallwar actually, the clippy-check action is already being used in our CI in the clippy.yml workflow.
We should probably either drop this file or remove the cargo check from the build_test_fmt.yml workflow.
I do not see any reason why we need both. What's your opinion on this @Skallwar @CohenArthur?

@CohenArthur
Copy link
Member

@Skallwar actually, the clippy-check action is already being used in our CI in the clippy.yml workflow. We should probably either drop this file or remove the cargo check from the build_test_fmt.yml workflow. I do not see any reason why we need both. What's your opinion on this @Skallwar @CohenArthur?

My understanding (but I might be wrong) is that the clippy-check action is used to create these nice looking warnings in the pull request window. However, we still want to run cargo clippy to prevent PRs from being merged should they introduce new clippy warnings

@CohenArthur
Copy link
Member

(or cargo check)

@tanguysegarra tanguysegarra force-pushed the fix-ci-using-clippy-instead-of-check branch from 9c819c1 to 0b5c133 Compare September 13, 2022 18:38
Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you :)

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.

Fix CI using clippy instead of check

4 participants