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

Improve passlib.apache #13689

Merged
merged 4 commits into from
Mar 21, 2025
Merged

Improve passlib.apache #13689

merged 4 commits into from
Mar 21, 2025

Conversation

donBarbos
Copy link
Contributor

No description provided.

This comment has been minimized.

This comment has been minimized.

@donBarbos
Copy link
Contributor Author

both check_password functions (in HtpasswdFile and HtdigestFile classes) might return None and it is even in their docstrings, like this:

    def check_password(self, user, password):
        """
        Verify password for specified user.
        If algorithm marked as deprecated by CryptContext, will automatically be re-hashed.

        :returns:
            * ``None`` if user not found.
            * ``False`` if user found, but password does not match.
            * ``True`` if user found and password matches.

        .. versionchanged:: 1.6
            This method was previously called ``verify``, it was renamed
            to prevent ambiguity with the :class:`!CryptContext` method.
            The old alias is deprecated, and will be removed in Passlib 1.8.
        """

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, one thing I noticed below.

Comment on lines 7 to 8
encoding: str | None
return_unicode: bool | None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While those are initialized to None in the class, they are immediately set in __init__, so they should never be None in instances:

Suggested change
encoding: str | None
return_unicode: bool | None
encoding: str
return_unicode: bool

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While those are initialized to None in the class, they are immediately set in __init__, so they should never be None in instances:

Unfortunately that means we need a stubtest_allowlist entry, but it is what it is.

This comment has been minimized.

Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

mitmproxy (https://github.com/mitmproxy/mitmproxy)
+ mitmproxy/addons/proxyauth.py:211: error: Incompatible return value type (got "bool | None", expected "bool")  [return-value]

@srittau srittau merged commit be52954 into python:main Mar 21, 2025
43 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.

2 participants