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

Fix issues from cargo clippy and add check in the verification #49

Open
dricair opened this issue Feb 2, 2025 · 4 comments
Open

Fix issues from cargo clippy and add check in the verification #49

dricair opened this issue Feb 2, 2025 · 4 comments

Comments

@dricair
Copy link
Contributor

dricair commented Feb 2, 2025

I see that cargo clippy returns quite a lot of warnings (8 at the moment) while there is benefit in maintaining the report clean.

I think we should:

  • Fix all warnings. I can do it as they are quite simple.
  • Add a pipeline check as verification when a pull request is proposed for merging. For that it would take more time for me as I don't know how to do it.
@isosphere
Copy link
Owner

I fixed a bunch here: 94eb9e3

I agree re: CI; I'm not super familiar with github actions but it should be simple enough for us to shove a "cargo clippy" command in there somewhere.

@dricair
Copy link
Contributor Author

dricair commented Feb 2, 2025

Ok I will rebase and verify.

@dricair
Copy link
Contributor Author

dricair commented Feb 2, 2025

I asked Claude AI this question:

On Yew Bootstrap Rust library, how can I add an action to run cargo clippy when someone proposes a pull request?

And it answered (Not verified):

name: Clippy Check

on:
  pull_request:
    branches: [ main ]

jobs:
  clippy:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      
      - uses: dtolnay/rust-toolchain@stable
        with:
          components: clippy
          
      - name: Run Clippy
        run: cargo clippy -- -D warnings

@dricair
Copy link
Contributor Author

dricair commented Feb 2, 2025

Actually it would make sense to add cargo test as well in the main package. Even if no tests exist at the moment (Although I am going to add some), there are at least all the tests on the examples for the documentation.

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

No branches or pull requests

2 participants