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

Add type annotations to utils and settings #519

Merged
merged 14 commits into from
Nov 15, 2019

Conversation

bhrutledge
Copy link
Contributor

Related to #231, following a similar process as #469.

This feels more substantial, and exposed some gotchas, particularly around username/password handling, for which I've left TODOs in the code.

Also, the type annotations are duplicating information in docstrings; I don't know if/how the latter should be updated.

I've found the coverage reports somewhat handy, though I don't fully understand them. Happy to tweak/document how those are generated (or remove them prior to merge).

I'm expecting this will cause merge conflicts with other PR's, especially #509.

With all that in mind, it seemed prudent to submit this for review before continuing.

Copy link
Member

@sigmavirus24 sigmavirus24 left a comment

Choose a reason for hiding this comment

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

Also, the type annotations are duplicating information in docstrings; I don't know if/how the latter should be updated.

That's probably best for now. The places that's done is for generated documentation (generated by Sphinx from the docstrings) and it's unclear to me if Sphinx can rely on annotations

twine/utils.py Outdated Show resolved Hide resolved
twine/utils.py Outdated Show resolved Hide resolved
@bhrutledge
Copy link
Contributor Author

Also, the type annotations are duplicating information in docstrings; I don't know if/how the latter should be updated.

That's probably best for now. The places that's done is for generated documentation (generated by Sphinx from the docstrings) and it's unclear to me if Sphinx can rely on annotations

I found https://github.com/agronholm/sphinx-autodoc-typehints, which looks promising at first glance. I'm inclined to open an issue (pending merge of this PR) to try it out. @sigmavirus24 What do you think?

@jaraco
Copy link
Member

jaraco commented Oct 30, 2019

I'm expecting this will cause merge conflicts with other PR's, especially #509.

Indeed it did, can you refresh based on the update?

@bhrutledge
Copy link
Contributor Author

Thanks, @jaraco. All set.

@sigmavirus24
Copy link
Member

I'm inclined to open an issue (pending merge of this PR) to try it out. @sigmavirus24 What do you think?

Yes, let's investigate that separately

@bhrutledge
Copy link
Contributor Author

@pypa/twine-maintainers How do y'all feel about merging this? It doesn't have an explicit approval yet.

@jaraco
Copy link
Member

jaraco commented Nov 9, 2019

I do agree this needs more attention. I'll try to find some time to do that.

tox.ini Show resolved Hide resolved
twine/repository.py Outdated Show resolved Hide resolved
twine/utils.py Outdated Show resolved Hide resolved
twine/utils.py Show resolved Hide resolved
twine/utils.py Outdated Show resolved Hide resolved
twine/repository.py Outdated Show resolved Hide resolved
@jaraco
Copy link
Member

jaraco commented Nov 11, 2019

I'd like to merge this, but it's pending review by @sigmavirus24, so I want to be considerate of that request.

@bhrutledge
Copy link
Contributor Author

@jaraco Thanks for taking care of the merge conflicts. It caught me a little by surprise, because I don't have notifications enabled for PR pushes (but maybe I should).

@bhrutledge
Copy link
Contributor Author

@jaraco Looks like there's a couple issues to sort out as a result of the merge. There's also a couple new functions that are missing type annotations. I'm happy to fix it up, hopefully in the next couple days.

@jaraco
Copy link
Member

jaraco commented Nov 14, 2019

That's right. I took a stab at merging, but only through the GitHub UI, so it's possible the merge was broken. Feel free to discard b508e70 and re-merge with master if that helps.

@bhrutledge bhrutledge removed the incomplete Work in progress label Nov 14, 2019
@bhrutledge
Copy link
Contributor Author

I've resolved the conflicts.

@pypa/twine-maintainers Unless there are any objections, I'm planning to squash & merge this tomorrow morning, so that I can continue adding type coverage.

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.

3 participants