Closed maerteijn closed 10 months ago
@Richardk2n I just formatted the files with black (there was indeed an error) and I squashed the commit.
The PR should pass flake8/black/isort now.
I don't have a Windows to test, but I think your test is failing, because a Windows path is not a valid regex.
In fact writing regexes for Windows paths would be rather annoying (requiring 4 \).
Maybe it would be sensible to parse the path to unix format instead and require that format in the setting (making the exclude setting portable in the process)?
I’ll see what I can do, I will update the PR soon(ish) and let you know!
@Richardk2n I've enabled the test runners on my fork now too so I could do a little "remote windows debugging" (aka print statements and look at the github actions output 😉).
Windows paths are indeed be a bit tricky with strings like c:\\a
because \a
will be seen as a special character and not as a literal character stream. With a raw string r"c:\\a"
you would solve this, but this can only be defined at parse time and is not usable for already assigned string variables. (You can't mark an existing string variable as a "raw string")
A way to match this (and other real unicode characters) is to encode all "special" characters with the unicode_escape
encoding and use this as input pattern for re.match
or re.search
:
>>> path = 'd:\\a\\pylsp-mypy\\pylsp-mypy\\test\\test_plugin.py'
>>> pattern = path.encode("unicode_escape").decode()
>>> pattern
'd:\\\\a\\\\pylsp-mypy\\\\pylsp-mypy\\\\test\\\\test_plugin.py'
It will match the original path:
>>> re.search(pattern, path)
<re.Match object; span=(0, 46), match='d:\\a\\pylsp-mypy\\pylsp-mypy\\test\\test_plugin.>
I implemented this in this PR and I added an extra test to verify the pattern matching. See these parts (which are included in the PR)
@Richardk2n Any chance you could take a look at my last response? 😇
Sorry for the delay.
If I understood correctly, what you implemented basically treats the pattern as a raw string?
This enables Windows users to just type Windows paths (while experienced users might incorrectly type the 4 \). This is good.
However, projects working on both Linux and Windows would need to specify each path twice with this solution, or did I misunderstand?
However, projects working on both Linux and Windows would need to specify each path twice with this solution
Good point.
I’d better implement this as you suggested earlier. This is also the way mypy itself implemented it: https://github.com/python/mypy/blob/bc591c756a453bb6a78a31e734b1f0aa475e90e0/mypy/modulefinder.py#L635
PR will be updated again soon 😉
Ok, updated the logic now that you can specify patterns with forward slashes /
and that these work on both windows and any unix variant:
def match_exclude_patterns(document_path: str, exclude_patterns: list) -> bool:
document_path = document_path.replace(os.sep, "/")
for pattern in exclude_patterns:
try:
if re.search(pattern, document_path):
log.debug(f"{document_path} matches " f"exclude pattern '{pattern}'")
return True
except re.error as e:
log.error(f"pattern {pattern} is not a valid regular expression: {e}")
return False
I also updated the unit test which now patches os.path
so this test is deterministic, independent of the testrunner os.
Bonus commit: mypy 1.7 was released with this PR: https://github.com/python/mypy/pull/15996
Which means that
TYPE_ERR_MSG = '"Dict[<nothing>, <nothing>]" has no attribute "append"'
changed into
TYPE_ERR_MSG = '"Dict[Never, Never]" has no attribute "append"'
I converted this into a regex so we support all versions of mypy.
Thank you for your contribution. I made the regex a little more specific and confused myself using windows paths again.
Please format the files using black.