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

Add code style checks #3

Closed
CMDJojo opened this issue Jan 19, 2023 · 4 comments · Fixed by #8 or #23
Closed

Add code style checks #3

CMDJojo opened this issue Jan 19, 2023 · 4 comments · Fixed by #8 or #23

Comments

@CMDJojo
Copy link
Member

CMDJojo commented Jan 19, 2023

It would be nice to have a rustfmt config so we all use the same formatting and add a CI tool that checks the style when doing a PR so we don't have badly formatted code in the main branch

@axoxelol1
Copy link
Collaborator

I messed around with some github actions and found some things that work. However, I've only done it with a single cargo project in the repository so I am not sure what the best way to have multiple crates be tested is. Maybe just write more actions or if the project is a cargo workspace we might be able to run the commands on all everything in the workspace. Unfortunately I do not know enough about rust project structures, or how we are structuring our project for that matter.

Here is an example that runs cargo check, cargo test, clippy, and rustfmt on pushes and PRs to main. If an action fails it will say what is wrong and how to fix it. https://gist.github.com/axoxelol1/786363790693f268838a798811b0a77d

@adelhult
Copy link
Collaborator

Your gist looks great, good work!

Looks like if we put everything in a workspace then we would just be able to run the CI action once (at least for the test suite, don't know how well this works for clippy and fmt though?)

We could also just repeat the command in each build but with different Cargo manifests specified

with:
       command: check
       args: --manifest-path=./playground/Cargo.toml

with:
       command: check
       args: --manifest-path=./core/Cargo.toml

# etc ...

However, I think an issue with this is that if for instance the "playground" build fails, the job would stop and we would not get to see diagnostics for "core" or any other crate. So maybe we want to copy the entire action and just have multiple jobs like you suggested. I guess this might end up costing a lot of computational minutes though, since we would have to run uses: actions-rs/toolchain@v1 for every every crate. But I guess that we just have to try and see how expensive it ends up being 😅.

I also took a look at this action that would allow us to host separate previews of the playground for each PR, but it might end up being a bit over-kill...

@CMDJojo
Copy link
Member Author

CMDJojo commented Jan 21, 2023

If I am not mistaken, you can use the -p option to run tasks on different packages, and I think you can specify it multiple times. It might work on sub-crates... In that way, we might be able to check formatting and tests on multiple crates on the same time.

@CMDJojo
Copy link
Member Author

CMDJojo commented Jan 21, 2023

But thank you for your gist! Will check it out tomorrow, and see if the -p option helps us out.

CMDJojo added a commit that referenced this issue Jan 22, 2023
CMDJojo added a commit that referenced this issue Jan 22, 2023
CMDJojo added a commit that referenced this issue Jan 22, 2023
CMDJojo added a commit that referenced this issue Jan 22, 2023
CMDJojo added a commit that referenced this issue Jan 22, 2023
CMDJojo added a commit that referenced this issue Jan 22, 2023
CMDJojo added a commit that referenced this issue Jan 22, 2023
CMDJojo added a commit that referenced this issue Jan 22, 2023
CMDJojo added a commit that referenced this issue Jan 22, 2023
CMDJojo added a commit that referenced this issue Jan 22, 2023
CMDJojo added a commit that referenced this issue Jan 22, 2023
CMDJojo added a commit that referenced this issue Jan 22, 2023
CMDJojo added a commit that referenced this issue Jan 22, 2023
CMDJojo added a commit that referenced this issue Jan 22, 2023
CMDJojo added a commit that referenced this issue Jan 22, 2023
@CMDJojo CMDJojo linked a pull request Feb 3, 2023 that will close this issue
CMDJojo added a commit that referenced this issue Feb 3, 2023
CMDJojo added a commit that referenced this issue Feb 3, 2023
CMDJojo added a commit that referenced this issue Feb 7, 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
3 participants