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 false symlink detection claims in verbose output #3385

Merged
merged 5 commits into from
Jan 19, 2023

Conversation

aaossa
Copy link
Contributor

@aaossa aaossa commented Nov 15, 2022

Description

Closes #3384

When trying to format a project from the outside, the verbose output shows says that there are symbolic links that points outside of the project, but displays the wrong project path. This behavior seem to be triggered when the command is executed from outside a project on a folder, causing an inconsistency between the path to the detected project root and the relative path to the target contents. The fix is to normalize the target path using the project root before processing the sources.

Checklist - did you ...

  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

When trying to format a project from the outside, the verbose output
shows says that there are symbolic links that points outside of the
project, but displays the wrong project path, meaning that these
messages are false positives.

This bug is triggered when the command is executed from outside a
project on a folder inside it, causing an inconsistency between the
path to the detected project root and the relative path to the target
contents.

The fix is to normalize the target path using the project root before
processing the sources, which removes the presence of the incorrect
messages.

Signed-off-by: Antonio Ossa Guerra <aaossa@uc.cl>
@aaossa
Copy link
Contributor Author

aaossa commented Nov 15, 2022

Seems like something is going on in Python 3.7 with the use of unittest.mock.call and its arguments, returning a tuple where I expect a string. I'll review that later. Also, updating CHANGES.md is pending.

EDIT: Seems like this is it:

# 3.7
>>> m = MagicMock(return_value=None)
>>> m(1, 2, 3, arg='one', arg2='two')
>>> kall = m.call_args
>>> args, kwargs = kall
>>> args
(1, 2, 3)

# 3.10
>>> m = MagicMock(return_value=None)
>>> m(1, 2, 3, arg='one', arg2='two')
>>> kall = m.call_args
>>> kall.args
(1, 2, 3)

The test attemps to emulate the behavior of the CLI as closely as
posible by patching some `pathlib.Path` methods and passing certain
reference paths to the context object and `black.get_sources`.

Before the associated fix was introduced, this test failed because
some of the captured files reported the presence of a symlink due to
an incorrectly formated path. The test also asserts that only a single
file is reported as ignored, which is part of the expected behavior.

Signed-off-by: Antonio Ossa Guerra <aaossa@uc.cl>
Add entry about fixing bug that cause false symlink detection messaged
in verbose output.

Signed-off-by: Antonio Ossa Guerra <aaossa@uc.cl>
@aaossa aaossa marked this pull request as ready for review November 15, 2022 16:12
@github-actions
Copy link

github-actions bot commented Nov 15, 2022

diff-shades reports zero changes comparing this PR (3b735bb) to main (d4ff985).


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

@aaossa
Copy link
Contributor Author

aaossa commented Jan 11, 2023

Hi, any updates on this?

Copy link
Collaborator

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

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

This looks good. I'm so sorry for dropping the ball on this!

We should fix and overhaul the file collection and discovery tests, it's not great that it's not testing typical usage. Could you open an issue? It's okay if you don't want to.

Thank you very much!

@ichard26 ichard26 merged commit 18fb884 into psf:main Jan 19, 2023
@aaossa aaossa deleted the issue-2424 branch January 20, 2023 15:33
@aaossa
Copy link
Contributor Author

aaossa commented Jan 20, 2023

This looks good. I'm so sorry for dropping the ball on this!

We should fix and overhaul the file collection and discovery tests, it's not great that it's not testing typical usage. Could you open an issue? It's okay if you don't want to.

Thank you very much!

No worries. About the file collection and discovery tests, I'll explore the code a bit first and open an issue this week. I think I red about a related problem it in a comment on another issue, but I'm not sure if its the same problem. I'll look for it and properly report these concerns

@ichard26
Copy link
Collaborator

ichard26 commented Feb 3, 2023

That would be great. Thank you! 🖤

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.

Output incorrectly claims symbolic links presence when formatting from outside a project
3 participants