Skip to content

check_parallel does not restore sys.path to the original value #7246

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

Closed
jacobtylerwalls opened this issue Jul 31, 2022 · 12 comments · Fixed by #7247
Closed

check_parallel does not restore sys.path to the original value #7246

jacobtylerwalls opened this issue Jul 31, 2022 · 12 comments · Fixed by #7247
Assignees
Labels
Milestone

Comments

@jacobtylerwalls
Copy link
Member

check_parallel does not restore sys.path to the original value.

Discovered while working on #7117, but unrelated to the issue it tested.

Opening an issue purely for the sake of satisfying the towncrier pre-commit check, which requires issues for all bugfixes.

@Pierre-Sassoulas
Copy link
Member

Opening an issue purely for the sake of satisfying the towncrier pre-commit check

Can't we use the PR number ?

@jacobtylerwalls
Copy link
Member Author

jacobtylerwalls commented Jul 31, 2022

I wondered, but we may not even merge #7117, and it was just a "found while developing" issue, not really related to it.

@Pierre-Sassoulas
Copy link
Member

I meant the number of the PR we open to fix the issue in this case #7247 (we do need to open it then create the towncrier fragment or hope for no other issue/PR to be opened before we create ours though).

@jacobtylerwalls
Copy link
Member Author

But then I have to no-verify my changes to get them up to Github. Opening an issue seemed lower friction.

@Pierre-Sassoulas
Copy link
Member

I have to no-verify my change

Is there a towncrier pre-commit check ? I often forgot to add the fragment myself I'd like this 😄

@jacobtylerwalls
Copy link
Member Author

Check newsfragments......................................................Failed
- hook id: check-newsfragments
- exit code: 1

doc/whatsnew/fragments/slug.bugfix: does not respect the standard format 🤖👎

The standard format is:

<one or more line of text>
<one blank line>
<issue reference> #<issuenumber>

Where <issue reference> can be one of: Refs, Closes, Follow-up in, Fixes part of

For example:

``pylint.x.y`` is now a private API.

Refs #1234

@jacobtylerwalls
Copy link
Member Author

Added in 54e54ad

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Jul 31, 2022

Ha right, but this does not check that the number is an issue or that it actually exists in github, only that the format is standard. So I think you can "cheat" 😄 (Or that there is a new fragment for your branch compared to main)

@jacobtylerwalls
Copy link
Member Author

Where can be one of: Refs, Closes, Follow-up in, Fixes part of

"Fixes part of too lazy to open an issue" 🤣

@Pierre-Sassoulas
Copy link
Member

"Fixes part of too lazy to open an issue" 🤣

🤣

It's funny because it's true. I'm pretty sure 50% of the time when I "reference" something it means this is a lazy pull request with no attached issue 😄

@DudeNr33
Copy link
Collaborator

The pre-commit check only triggers if you have created a new file in the newsfragments folder.
You can use the PR number if you'd like, but you will have to push at least twice (as you don't know the PR number beforehand).

I'm open to ideas how to improve this! 😊

@Pierre-Sassoulas
Copy link
Member

It was mostly just a discussion on the benefit of creating a small issue vs a two stage PR, both are fine and we don't need to change anything imo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants