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

Fix KWallet.get_credential() regression #463

Merged
merged 3 commits into from Sep 12, 2020

Conversation

MrAnno
Copy link
Contributor

@MrAnno MrAnno commented Sep 1, 2020

#459 introduced a regression.

Docs:

get_credential(service, username): Return a credential object stored in
the active keyring. This object contains at least username and password
attributes for the specified service, where the returned username may be
different from the argument.

Prior to this commit, get_credential() returned the password as a string, without wrapping the credential into a SimpleCredential object.

Docs:
get_credential(service, username): Return a credential object stored in
the active keyring. This object contains at least username and password
attributes for the specified service, where the returned username may be
different from the argument.

Prior to this commit, get_credential() returned the password as a string,
without wrapping the credential into a SimpleCredential object.
@muggenhor
Copy link
Contributor

@MrAnno thanks for catching my mistake and fixing it 😊

@MrAnno
Copy link
Contributor Author

MrAnno commented Sep 8, 2020

@muggenhor My pleasure :)

IMHO your version of the fix is more straightforward and explicit.

@jaraco jaraco merged commit 94a1ef6 into jaraco:master Sep 12, 2020
@jaraco
Copy link
Owner

jaraco commented Sep 12, 2020

Would be good to get some enhancements to the test suite to capture this guarantee. Just adding some type hints might be sufficient.

jaraco added a commit that referenced this pull request Sep 12, 2020
… was hoping adding these hints would capture the missed expectation in #463, but alas, they do not.
@jaraco
Copy link
Owner

jaraco commented Sep 12, 2020

As you can see in the above commit, I've added the type hints, but it seems mypy is comfortable with that.

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

3 participants