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 Rust CI #237

Merged
merged 2 commits into from
Jul 17, 2022
Merged

Add Rust CI #237

merged 2 commits into from
Jul 17, 2022

Conversation

AntoineRR
Copy link
Collaborator

Description

  • Fix Rust clippy warnings
  • Add a CI for the Rust part of the project, running cargo check, cargo test, cargo fmt, and cargo clippy

Notes

  • I added more than just clippy, let me know if you want to remove some stuff
  • There are two different CI systems in the project : Github Actions and Circle CI. I added the Rust CI to the Github Actions CI because that's what I am the most familiar with. Is there a reason why Circle CI is used for testing the Python part of Robyn? If you prefer having the Rust CI on Circle CI I think I can figure it out and move it there :)

This PR fixes #236

@netlify
Copy link

netlify bot commented Jul 16, 2022

Deploy Preview for robyn canceled.

Name Link
🔨 Latest commit 89ff862
🔍 Latest deploy log https://app.netlify.com/sites/robyn/deploys/62d2c8869cd97a0009c68815

@AntoineRR AntoineRR changed the title Clippy ci Add Rust CI Jul 16, 2022
Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

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

Thank you for the contributions! :D

I have an inline comment.

Comment on lines +5 to +32
jobs:
check:
name: Check
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: stable
override: true
- uses: actions-rs/cargo@v1
with:
command: check

test:
name: Test Suite
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: stable
override: true
- uses: actions-rs/cargo@v1
with:
command: test
Copy link
Member

Choose a reason for hiding this comment

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

@AntoineRR , are these the replacements of the circle ci?

These are cargo test and cargo check , right?

cargo test is already being done in the .circleci/config.yml file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for your feedback :)

From what I understand of CircleCI, you indeed defined a test command in .circleci/config.yml on line 12, that would run cargo test. However, this command is currently not used in any of the workflows. To use the test command, it should be added under steps inside a job (example from the doc: the command sayhello is invoked inside the myjob job, which has to be run by a workflow). As I am not very familiar with this, I may have misunderstood something, so feel free to correct me if that's the case.

I also chose to add the commands to github workflows as it is easier to specify the CI should run on push AND on pull requests. To move cargo test, cargo fmt and cargo clippy to circle CI, I think I would just have to uncomment lines 56-57 of .circleci/config.yml to invoke the lint-test-build job defined in the rust orb :) Let me know what suits you best!

Copy link
Member

Choose a reason for hiding this comment

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

@AntoineRR , that makes sense. I anyway want to move away from CircleCI. This will be a good first step.

Thanks!

Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@sansyrox sansyrox merged commit 500e261 into sparckles:main Jul 17, 2022
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.

[Feature Request] add clippy in ci
2 participants