-
Notifications
You must be signed in to change notification settings - Fork 803
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: Further updates to Contributing.md re testing, linting etc. #4115
base: main
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #4115 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really useful! Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'm definitely in favor of the docs changes, they look great!
But I'm not so sure how I feel about the CI workflow. (I don't have much experience with GH actions, so if I got something wrong, please correct me.)
I assume the workflow is intended to run in forks? Personally I run most of these checks already locally, so for me I would see no need to let them run again in CI on my fork. Most failures I observe originate from feature combinations or Python versions, which are not checked here. I guess it is possible to disable actions on forks that don't want this, but generally I would prefer something like this to be opt-in, rather than opt-out (not sure if that's possible).
This would also run on other branches in the pyo3
main repo (I assume), not sure if that's desired.
Thanks @Icxolu, I'm glad you like the doc adjusments. Yes the idea is that the workflow would run in forks, and yes, it would also run on other branches in the I take the point of there needing to be an ability to opt-in/out - the simplest way to do this (for both the user and in terms of pipeline complexity) would be based on branch naming: Personally I like to offload as much as possible to a pipeline. Given that github actions are effectively free test execution while you can get on with your next adjustments I don't see the disadvantage in having the pipeline run by default - as long as the workflow is kept to a suitable max duration (my pain point is about 200s). It even has the advantage of ensuring all contributors think about fmt, clippy and integration tests as they go, not just when they raise a PR. But I'm open to an opt-in approach if that better suits the views of the contributors here. FWIW: I also have a very commit-heavy approach compared to most developers, making a very tiny change; running a few quick tests after each change (in rust that's usually just unit tests in the file I just changed), with feedback in a few seconds; then pushing a bundle of 2-5 commits and letting the pipeline handle the rest - I don't mind waiting 2-3 minutes at this point for final feedback. |
They are a waste of computing resources which run on scarce natural resources if they run unconditionally and redundantly to a local compile-test-edit cycle. I think our current stance here is that the Nox sessions are meant to reproduce as much of the CI locally as is necessary to push changes with reasonable confidence that they pass the pull request CI. Personally, I would rather invest into making this work than nudging contributors into a centralized GitHub-dependent workflow. |
Hi @Icxolu I have check running in IDE - how do you get clippy to run there, that would be really useful? I hadn't noticed I don't think any additional I don't want to be that guy who turns up and in his first couple of contributions reformats the code-base because "my way is better". At the same time I feel emotionally triggered by the expectation to clean up my accomodations as they are bad for the environment and not something the community wishes to promote. I'm OK to conclude that this probably isn't a codebase I will spend much time working on, after all we're doing this for fun so it needs to be a good fit on both sides. |
@MusicalNinjaDad @adamreichold I see where you're both coming from here. Balancing these different needs and goals seems difficult, but I'd like to find a way forward that allows @MusicalNinjaDad to continue to contribute and feel welcome. |
Building on #4080 this further improves the detail and structure of contributing.md regarding testing, linting, CI etc. by separately detailling what is run in PR CI and useful command to run in a dev environment.
Furthermore this introduces a quick-CI workflow which runs in approx 3 minutes on all branches except main. This allows contributors to run the most necessary tests locally, offload larger testing efforts to github and still have the confidence that every push will undergo suitable regression testing with quick feedback.