Skip to content

create_baseline_stubs.py: Fix path separator on windows and ignore root /out #8653

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 2, 2022
Merged

create_baseline_stubs.py: Fix path separator on windows and ignore root /out #8653

merged 4 commits into from
Sep 2, 2022

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Aug 31, 2022

On windows, the stub path is generated with a backslash, which of course is not valid when added to the json file.
create_baseline_stubs.py also creates a out folder at the root which was not gitignored.

@Avasam Avasam force-pushed the create_baseline_stubs-fixes branch from 729e926 to de3e9c8 Compare August 31, 2022 00:04
@Avasam Avasam force-pushed the create_baseline_stubs-fixes branch from de3e9c8 to 2cdb3ae Compare August 31, 2022 00:04
@AlexWaygood AlexWaygood changed the title Fixed path separator on windows and ignore root /out create_baseline_stubs.py: Fix path separator on windows and ignore root /out Aug 31, 2022
AlexWaygood
AlexWaygood reviewed Aug 31, 2022
View reviewed changes
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks! I remember being bitten by the path-separator thing as well when I used create_baseline_stubs.py a while back on my Windows machine. Can't remember why I didn't file a PR to fix it.

hauntsaninja
hauntsaninja reviewed Aug 31, 2022
View reviewed changes
Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Maybe we should do a replace in add_pyright_exclusion and let everything else do normal os.path stuff?

@AlexWaygood
Copy link
Member

Maybe we should do a replace in add_pyright_exclusion and let everything else do normal os.path stuff?

SGTM, though everything seems to work fine on my Windows machine either way :)

@Avasam
Copy link
Collaborator Author

Avasam commented Sep 1, 2022 •
edited
Loading

Maybe we should do a replace in add_pyright_exclusion and let everything else do normal os.path stuff?

SGTM, though everything seems to work fine on my Windows machine either way :)

Everything works in windows / powershell for me as well with either slashes (well except setting the environment variable MYPYPATH since the syntax is different in powershell, but that's unrelated to this work). So I don't mind either way. Only changing the slashes in add_pyright_exclusion might be more consistent in code, instead of having a single exception to using path.join

@AlexWaygood
Copy link
Member

Only changing the slashes in add_pyright_exclusion might be more consistent in code, instead of having a single exception to using path.join

Yes, let's do that

AlexWaygood
AlexWaygood approved these changes Sep 2, 2022
View reviewed changes
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM!

@AlexWaygood AlexWaygood mentioned this pull request Sep 2, 2022
Merged
@hauntsaninja hauntsaninja merged commit bfe56cd into python:master Sep 2, 2022
@Avasam Avasam deleted the create_baseline_stubs-fixes branch September 2, 2022 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Akuli Akuli Akuli left review comments

@hauntsaninja hauntsaninja hauntsaninja approved these changes

@AlexWaygood AlexWaygood AlexWaygood approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Avasam @AlexWaygood @hauntsaninja @Akuli