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

Lock pydocstyle to <6.2. #38

Merged
merged 4 commits into from
Feb 19, 2023

Conversation

real-yfprojects
Copy link
Collaborator

@real-yfprojects real-yfprojects commented Feb 14, 2023

Pydocstyle is a pylama dependency. However pylama doesn't support the newest pydocstyle version. See klen/pylama#232.

  • poetry.lock: Update.

  • pyproject.toml: Lock pydocstyle version.

  • update isort hook

Pydocstyle is a pylama dependency. However pylama doesn't support the newest pydocstyle version. See klen/pylama#232.

* poetry.lock: Update.

* pyproject.toml: Lock pydocstyle version.
Isort must be updated since the latest poetry version rejects its configuration. See #2077.
@johannesjh
Copy link
Owner

Hi @real-yfprojects, thank you for submitting this update!

  • updating isort: thank you, looks alright
  • locking pydocstyle to <6.2: we discussed this in Poetry hook #36, ok to me.

@johannesjh
Copy link
Owner

I checked out your branch and ran pre-commit run --all ...this worked fine locally on my machine. but the pull request has one failed pipeline

pylama...................................................................Failed
- hook id: pylama
- exit code: 1

req2flatpak.py:147:17 W0719 Raising too general exception: Exception [pylint]
req2flatpak.py:155:17 W0719 Raising too general exception: Exception [pylint]
req2flatpak.py:322:35  Incompatible default for argument "minor_version" (default has type "None", argument has type "int")  [assignment] [mypy]
req2flatpak.py:322:35  PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True [mypy]
req2flatpak.py:322:35  Use https://github.com/hauntsaninja/no_implicit_optional to automatically upgrade your codebase [mypy]
req2flatpak.py:347:35  Incompatible default for argument "minor_version" (default has type "None", argument has type "int")  [assignment] [mypy]
req2flatpak.py:347:35  PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True [mypy]
req2flatpak.py:347:35  Use https://github.com/hauntsaninja/no_implicit_optional to automatically upgrade your codebase [mypy]

any idea why? I am surprised to see that since it worked fine locally.

@real-yfprojects
Copy link
Collaborator Author

Pylama fails on my machine too. Did you sync your poetry environment with the lock file?

@johannesjh
Copy link
Owner

johannesjh commented Feb 19, 2023

I reviewed the errors from the failed linting pipeline... their critique is legitimate. It boils down to the following three problems that are luckily easy to fix:

1.) To solve req2flatpak.py:147:17 W0719 Raising too general exception: Exception [pylint], we can redefine InvalidWheelFilename to be a more specific exception, as in the original code from the packaging package:

# we should use this definition:
class InvalidWheelFilename(ValueError):
    """An invalid wheel filename was found, users should refer to PEP 427."""

# instead of:
InvalidWheelFilename = Exception

2.) To solve req2flatpak.py:322:35 Incompatible default for argument "minor_version" (default has type "None", argument has type "int") [assignment] [mypy], we should define the type of the minor_version argument as Optional[int], like so:

@classmethod
def from_python_version_and_arch(
    cls, minor_version: Optional[int] = None, arch="x86_64"
) -> Platform:

3.) The same solution applies to req2flatpak.py:347:35 Incompatible default for argument "minor_version" (default has type "None", argument has type "int") [assignment] [mypy], i.e., we should also define the type to be Optional[int].

@classmethod
def _cp3_linux_tags(
    cls, minor_version: Optional[int] = None, arch="x86_64"
) -> Generator[str, None, None]:

...the above changes should make pylint and mypy happy again... can you implement them in your branch to get the CI green again?

@real-yfprojects
Copy link
Collaborator Author

the above changes should make pylint and mypy happy again... can you implement them in your branch to get the CI green again?

Of course. I was wondering though, why the ci didn't fail for previous commits/PRs. I didn't introduce these linting errors.

@johannesjh
Copy link
Owner

Did you sync your poetry environment with the lock file?

thank you for pointing it out... that was the missing step :-)

@johannesjh
Copy link
Owner

johannesjh commented Feb 19, 2023

I was wondering though, why the ci didn't fail for previous commits/PRs. I didn't introduce these linting errors.

my guess is that this has to do with updated dependencies. your pull request updates mypy. and mypy's output states explicitely that they changed their behavior to forbid implicit optionals:

req2flatpak.py:322:35  Incompatible default for argument "minor_version" (default has type "None", argument has type "int")  [assignment] [mypy]
req2flatpak.py:322:35  PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True [mypy]

@real-yfprojects
Copy link
Collaborator Author

Ah yes, pylint also introduced new checks with 2.16.0.

@real-yfprojects
Copy link
Collaborator Author

What do you think about sync_with_poetry?

* .pre-commit-config.yaml
* poetry.lock: Updated by `poetry-lock` hook.
* req2flatpak.py : Add `InvalidWheelFilename` exception.

* req2flatpak.py: Adjust type hints of `from_python_version_and_arch` and `_cp3_linux_tags`.
@johannesjh
Copy link
Owner

What do you think about sync_with_poetry?

looks good! (I always found it weird to maintain different sets of package versions for pre-commit hooks versus other dev dependencies). if you are willing to give it a try, go at it!

@real-yfprojects real-yfprojects merged commit c987f84 into johannesjh:main Feb 19, 2023
@real-yfprojects real-yfprojects deleted the lock-pydocstyle branch February 19, 2023 08:13
@johannesjh johannesjh added this to the v0.2 milestone Apr 5, 2023
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.

None yet

2 participants