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

Fix handling of Windows junctions in normalize_path_maybe_ignore (#2569) #2904

Merged
merged 3 commits into from Mar 8, 2022
Merged

Fix handling of Windows junctions in normalize_path_maybe_ignore (#2569) #2904

merged 3 commits into from Mar 8, 2022

Conversation

yoerg
Copy link
Contributor

@yoerg yoerg commented Mar 3, 2022

Description

Fixes #2569: error on windows directory junctions

Thanks @pmolodo for the excellent analysis.

Checklist - did you ...

  • Add a CHANGELOG entry if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

@github-actions
Copy link

@github-actions github-actions bot commented Mar 3, 2022

diff-shades reports zero changes comparing this PR (6cb749f) to main (6f4976a).


What is this? | Workflow run | diff-shades documentation

# `child` should behave like a strange file which resolved path is clearly
# outside of the `root` directory.
child.is_symlink.return_value = False
with pytest.raises(ValueError):
Copy link
Contributor Author

@yoerg yoerg Mar 4, 2022

Choose a reason for hiding this comment

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

So this is a slight behavioral change: black will no longer raise an exception for these strange files (e.g. Windows directory junctions), ignoring them just like symlinks. I don't know of any other strange links, but I can't imagine a benefit in stopping the analysis for any of them.

@yoerg
Copy link
Contributor Author

@yoerg yoerg commented Mar 7, 2022

I have no idea why diff-shades failed but it doesn't seem to be related to these changes. @JelleZijlstra or some other maintainer, can you help me out please?

@ichard26
Copy link
Collaborator

@ichard26 ichard26 commented Mar 7, 2022

I've triggered a rerun, let's see if that fixes it. Sorry for the hiccup!

@yoerg
Copy link
Contributor Author

@yoerg yoerg commented Mar 8, 2022

Thanks, that did it!

The PR is ready.

Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Thanks for your contribution!

@JelleZijlstra JelleZijlstra merged commit 24ffc54 into psf:main Mar 8, 2022
38 checks passed
@ichard26
Copy link
Collaborator

@ichard26 ichard26 commented Mar 8, 2022

Thank you so much for your contribution! This project is only possible by contributions like these 🖤. You're awesome, @yoerg -- path handling is surprisingly tricky sometimes. If you got any feedback to share let us know in #2238 ^^

@pmolodo
Copy link

@pmolodo pmolodo commented Mar 8, 2022

Huzzah! Thanks @yoerg!

@yoerg yoerg deleted the fix-2569 branch Mar 8, 2022
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.

4 participants