Extract auth options resolver #537
Conversation
…ct-oriented techniques to avoid proliferation of function parameters.
There was a problem hiding this comment.
I like this direction. Here's some initial feedback and suggestions. Also, when I run the tests, I get prompted for a password:
$ tox -e py37
...
py37 run-test: commands[0] | coverage run --source twine -m pytest tests
============================= test session starts ==============================
platform darwin -- Python 3.7.3, pytest-5.2.4, py-1.8.0, pluggy-0.13.0
cachedir: .tox/py37/.pytest_cache
rootdir: /Users/brian/Code/twine, inifile: pytest.ini
plugins: services-2.0.1
collected 104 items
tests/test_auth.py Enter your password:
That's the weird thing. I'm seeing that too (what I previously characterized as a deadlock), but only after merging with the latest master, and only sometimes does the prompt appear (other times, the text from the prompt is missing, but it still blocks). |
Co-Authored-By: Brian Rutledge <[email protected]>
…interactive" behavior.
And in f50efaa, I've corrected the issue. What's really strange and I still don't understand is why were the tests passing on earlier commits, before the typing adjustments. |
…ettings' object. This change requires the Resolver to be aware of internal details on the object, but simplifies the interface and interactions with Settings objects.
| self.auth = auth.Resolver.choose(not non_interactive)( | ||
| self.repository_config, | ||
| auth.CredentialInput(username, password), | ||
| ) |
| except Exception as exc: | ||
| warnings.warn(str(exc)) | ||
| return None # TODO: mypy shouldn't require this | ||
| return None # any more than it should require this |
There was a problem hiding this comment.
Why are there two returns here? I'm surprised this passed linting.
There was a problem hiding this comment.
Yeah, that's me being a little pedantic in reaction to mypy being unnecessarily pedantic. Mypy requires the first return None, which in my mind is redundant lint. I learned over 20 years ago that a Python function that reaches the end of the block without returning anything has an implicit return None, but now mypy is violating that language construct by requiring an explicit return.
| def system(self) -> Optional[str]: | ||
| return self.config['repository'] | ||
|
|
||
| def get_username_from_keyring(self): |
There was a problem hiding this comment.
This could still use type annotations.
In #489, I raised a concern:
This pull request implements that refactoring. As you can see, the 'non_interactive' boolean parameter now is passed into one function and causes its effect there. This change uses many fewer lines to enact the same work and in many ways provides a cleaner abstraction around authentication handling.
Unfortunately, although the tests pass against the commit where I made the change, when I tried to merge that change with the static typing code, it now deadlocks in the tests. I'll take another look at this at some point.
Edit: my first attempt at this refactoring is at d7ff2c8