Skip to content

mypy: fix "unused type ignore" issue on windows#5524

Merged
branchv merged 1 commit into
python-poetry:masterfrom
radoering:fix-mypy-for-windows
May 1, 2022
Merged

mypy: fix "unused type ignore" issue on windows#5524
branchv merged 1 commit into
python-poetry:masterfrom
radoering:fix-mypy-for-windows

Conversation

@radoering
Copy link
Copy Markdown
Member

At the moment the mypy pre-commit hooks fails on windows with:

src\poetry\utils\appdirs.py:211: error: Unused "type: ignore" comment

@radoering radoering force-pushed the fix-mypy-for-windows branch from 4eb7c98 to c7c6817 Compare April 30, 2022 18:15
@radoering radoering requested a review from a team April 30, 2022 18:23
@radoering radoering force-pushed the fix-mypy-for-windows branch from c7c6817 to 33ce698 Compare April 30, 2022 19:40
@dimbleby
Copy link
Copy Markdown
Contributor

poetry.utils._compat has WINDOWS = sys.platform == "win32". With that in mind I am pretty suspicious about whether the more elaborate definition in this file is actually useful.

It looks as though sys.platform == "cli" was a thing IronPython did, but not since IronLanguages/ironpython3#395, which seems to have gone into their version 3.4.0 - so that surely is not something we need to worry about any more.

Therefore suggest it would be cleaner to change all of the if WINDOWS in this file to if sys.platform == "win32", and also put everything from the definition of _get_win_folder_from_registry() downwards inside an if sys.platform == "win32" block? That would satisfy mypy I think.

@branchv
Copy link
Copy Markdown
Member

branchv commented May 1, 2022

it would be cleaner to change all of the if WINDOWS in this file to if sys.platform == "win32"

agreed, this code originated from appdirs, which has been superseded by platformdirs which does exactly that

https://github.com/platformdirs/platformdirs/blob/60fc40c294d90f7687f779487b8c19679fb73255/src/platformdirs/__init__.py#L20-L21

@radoering radoering force-pushed the fix-mypy-for-windows branch from 33ce698 to 21b59fd Compare May 1, 2022 05:26
@dimbleby
Copy link
Copy Markdown
Contributor

dimbleby commented May 1, 2022

platformdirs is already in poetry's dependency tree (via virtualenv), perhaps it would be better just to use that directly, rather than maintaining an old copy of it

@radoering
Copy link
Copy Markdown
Member Author

You are probably right but that requires further investigation. The docstring says "modified to suit our purposes". I suppose we should check what modifications were made and if these are obsolete when using platformdirs.

For now, I'm satisfied with fixing mypy but feel free to check if platformdirs can be used safely and draft a PR.

@dimbleby
Copy link
Copy Markdown
Contributor

dimbleby commented May 1, 2022

#5527

per comment in that MR, this does change the behaviour on MacOS, in a way that seems to be a straight bug fix. I look forward to learning whether poetry feels able to take such things.

@branchv branchv merged commit bf04e20 into python-poetry:master May 1, 2022
@kasteph kasteph mentioned this pull request May 30, 2022
@github-actions
Copy link
Copy Markdown

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Feb 29, 2024
@radoering radoering deleted the fix-mypy-for-windows branch November 24, 2024 12:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants