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

docs: Make contributing.md slightly more clear for newer contributors #4080

Merged
merged 4 commits into from Apr 16, 2024

Conversation

dmatos2012
Copy link
Contributor

@dmatos2012 dmatos2012 commented Apr 14, 2024

Hello dear maintainers:

Based on my new experience contributing for the 1-2 time, I think the following additions on the Contributing.md could help newer contributors to know exactly what commands they should execute to sort of check their PR before CI flags it. Being more explicit over just waiting for the CI to tell you has the advantage of:

  • More PRs not failing tests + linting
  • Make contributor aware of specifically what tools the repo is using to check for correctness.
  • Making sure their new/change feature does not break current functionality. Specifically, i mean that just running nox or cargo test doesnt run test code for --features, but the CI does, thus a contributor would assume everything goes alright until its flagged by CI and the "angry" maintainer checks that such a basic check wasn't done by the contributor. :).

These are just my 2 cents, thanks for the great repo :).

Edit: Since I am not familiar at all with the repo, I assume there are other tips and tricks that are checked in CI and for the code, so I could gladly add them if pointed out that its missed.

@LilyFoote
Copy link
Contributor

I'm definitely in favour of better guidance on which tests to run locally. I usually just do the basic nox -s test and leave the rest to CI, but I'm sure this isn't the most efficient approach.

Contributing.md Outdated Show resolved Hide resolved

Formatting, linting and tests are checked for all Rust and Python code. In addition, all warnings in Rust code are disallowed (using `RUSTFLAGS="-D warnings"`).
#### Linting Python code
Copy link
Member

Choose a reason for hiding this comment

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

While the overview of possible commands is nice, I think it should go after line 104 (in the original file).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree. Have moved it there

@dmatos2012
Copy link
Contributor Author

I'm definitely in favour of better guidance on which tests to run locally. I usually just do the basic nox -s test and leave the rest to CI, but I'm sure this isn't the most efficient approach.

Yeah but I feel thats what bit me, because I was changing code in rust_decimal which is only run if you manually run cargo test --features rust_decimal or if you wait for CI, thus sometimes nox -s test might not be enough

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks, this is a good idea! Some other commands I use relatively often:

# to check all conditional compilation
nox -s check-feature-powerset

# to update UI tests
nox -s update-ui-tests

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks!

@davidhewitt davidhewitt added this pull request to the merge queue Apr 16, 2024
Merged via the queue into PyO3:main with commit 2ad2a3f Apr 16, 2024
43 checks passed
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.

None yet

4 participants