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

Fixing pre-commit on Windows (disable warns for unused ignores) #1096

Open
vidartf opened this issue Nov 25, 2022 · 1 comment
Open

Fixing pre-commit on Windows (disable warns for unused ignores) #1096

vidartf opened this issue Nov 25, 2022 · 1 comment

Comments

@vidartf
Copy link
Member

vidartf commented Nov 25, 2022

Problem

  1. The mypy type checks for jupyter_server are currently *nix only. E.g. it will fail when trying to access socket.AF_UNIX when on windows. Using assert sys.platform != 'win32' as recommended for platform specific code doesn't seem to work?
  2. For the Windows specific code, we have type: ignore comments on them. Since we use a strict config, this will fail with error: Unused "type: ignore" comment on Windows. Note that it is not currently possible to ignore "unused ignore" errors: Cannot silence "unused 'type: ignore'" in version specific code python/mypy#8823
  3. The out-of-the-box pre-commit integrations with git do not work on Windows: Add sys.executable escapes on Windows pre-commit/pre-commit#2613 (this can be fixed manually by each user by editing the git hooks file)
  4. If you use the core.autocrlf=true setting, some linters (e.g. mdformat) will aggressively "fix" line endings, although git will subsequently fix it back when you do git add.

Proposed Solution

For 1. and 2.:

  • Add type: ignore comments on all OS-specific bits, and use flag warn_unused_ignores = False.
  • Run mypy on Windows CI as well to avoid future breakages.

For 3., hopefully the PR gets accepted and released. If not, we can maybe add a separate instruction to the docs.

For 4., we can maybe add another run of end-of-line-fixer to the end of the pre-commit config? That way any mess that e.g. the mdformat extension makes will be rectified again. Not really critical, but nice-to-have.

@blink1073
Copy link
Contributor

blink1073 commented Nov 25, 2022

Using assert sys.platform != 'win32' as recommended for platform specific code doesn't seem to work?

Does it work if you use an if : guard instead?

I think it's fine to run the end-of-line fixer last

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants