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

Support alternate tox.ini locations. #25

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

jsirois
Copy link
Contributor

@jsirois jsirois commented Aug 18, 2023

No description provided.

@jsirois jsirois requested review from benjyw and cognifloyd August 18, 2023 16:43
@jsirois
Copy link
Contributor Author

jsirois commented Aug 18, 2023

This is being used successfully in this branch: pex-tool/pex#2198

@cognifloyd cognifloyd merged commit b16b9cf into pantsbuild:main Aug 18, 2023
@jsirois jsirois deleted the tox/fix-cwd-assumption branch August 18, 2023 19:59
@jsirois
Copy link
Contributor Author

jsirois commented Aug 18, 2023

Thanks for the review @cognifloyd. I think maybe the general Pants community thinks merging someone else's PR is OK, and in this case it was, but, in general, this seems very much not cool to do. I know I often have later thoughts and make adjustments before landing.

@cognifloyd
Copy link
Member

cognifloyd commented Sep 6, 2023

@jsirois if you are actively working on a PR then you must include WIP in the title, or mark it as draft (a core GitHub feature). Once it is out of draft, then any maintainer can and SHOULD merge to avoid PRs piling up.

I have been a mere contributor (not a maintainer) on many projects where my PR sat approved but unmerged for months because the maintainers were used to the closed source practice of PR authors merging their own PR. In open source projects, the last maintainer that approves is responsible for merging to ensure that doesn't happen.

@jsirois
Copy link
Contributor Author

jsirois commented Sep 6, 2023

Ok, I hate that idea (for maintainers, makes sense for merging non maintainer PRs of course!). But at least I know your reasoning.

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.

2 participants