Skip to content

Fix false symlink detection claims in verbose output#3385

Merged
ichard26 merged 5 commits into
psf:mainfrom
aaossa:issue-2424
Jan 19, 2023
Merged

Fix false symlink detection claims in verbose output#3385
ichard26 merged 5 commits into
psf:mainfrom
aaossa:issue-2424

Conversation

@aaossa

@aaossa aaossa commented Nov 15, 2022

Copy link
Copy Markdown
Contributor

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

aaossa commented Nov 15, 2022

Copy link
Copy Markdown
Contributor Author

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

github-actions Bot commented Nov 15, 2022

Copy link
Copy Markdown
Contributor

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


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

@aaossa

aaossa commented Jan 11, 2023

Copy link
Copy Markdown
Contributor Author

Hi, any updates on this?

@ichard26 ichard26 left a comment

Copy link
Copy Markdown
Collaborator

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

aaossa commented Jan 20, 2023

Copy link
Copy Markdown
Contributor Author

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

ichard26 commented Feb 3, 2023

Copy link
Copy Markdown
Collaborator

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

3 participants