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

Add exclude option to pylsp-mypy configuration #71

Merged
merged 10 commits into from
Nov 15, 2023

Conversation

maerteijn
Copy link

No description provided.

@Richardk2n
Copy link
Member

Please format the files using black.

@maerteijn
Copy link
Author

@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.

@Richardk2n
Copy link
Member

Richardk2n commented Oct 18, 2023

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)?

@maerteijn
Copy link
Author

I’ll see what I can do, I will update the PR soon(ish) and let you know!

@maerteijn maerteijn force-pushed the feature/exclude branch 3 times, most recently from 47f3cfe to 6168d45 Compare October 21, 2023 16:55
@maerteijn maerteijn force-pushed the feature/exclude branch 2 times, most recently from 00226fa to 10cbf6b Compare October 21, 2023 21:03
@maerteijn
Copy link
Author

maerteijn commented Oct 21, 2023

@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)

@maerteijn
Copy link
Author

@Richardk2n Any chance you could take a look at my last response? 😇

@Richardk2n
Copy link
Member

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?

@maerteijn
Copy link
Author

maerteijn commented Nov 8, 2023

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 😉

Just convert windows paths to unix (a ilke) paths, so d:\My Documents\my_file.py becomes d:/My Documents/my_file.py

Then you can reuse the configured exclude patterns for both windows and unix (a like) platforms.

fds
@maerteijn
Copy link
Author

maerteijn commented Nov 15, 2023

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: python/mypy#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.

@Richardk2n
Copy link
Member

Richardk2n commented Nov 15, 2023

Thank you for your contribution.
I made the regex a little more specific and confused myself using windows paths again.

@Richardk2n Richardk2n merged commit 2f569f6 into python-lsp:master Nov 15, 2023
9 checks passed
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.

3 participants