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

Prints the packages that will be uploaded when using the --verbose option #652

Conversation

VikramJayanthi17
Copy link
Contributor

As per discussion in #381, twine now prints the names of the packages that it will attempt to upload when the --verbose option is selected. The example output below is from the terminal on my MacOs(Mojave) machine.

Screen Shot 2020-06-05 at 2 44 11 PM

This change was suggested by @bhrutledge as a simple first change to the --verbose option using the existing verbose pattern before more comprehensive changes are implemented using the stdlib logging system.

@sigmavirus24
Copy link
Member

Please fix your failing tests https://travis-ci.org/github/pypa/twine/builds/695224649 and clean up your commit history on the branch

@bhrutledge bhrutledge self-requested a review June 6, 2020 21:30
Copy link
Contributor

@bhrutledge bhrutledge left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, and providing the screenshot.

I also agree re: a clean commit history. Here are some resources:

twine/commands/upload.py Outdated Show resolved Hide resolved
twine/commands/upload.py Outdated Show resolved Hide resolved
@VikramJayanthi17 VikramJayanthi17 force-pushed the testing-add-uploaded-packages-to-verbose branch from 9b10a33 to a3ec4c1 Compare June 8, 2020 17:56
@di di requested a review from bhrutledge June 8, 2020 19:34
@VikramJayanthi17 VikramJayanthi17 force-pushed the testing-add-uploaded-packages-to-verbose branch from d51d02f to 5775bbc Compare June 8, 2020 21:38
Copy link
Contributor

@bhrutledge bhrutledge left a comment

Choose a reason for hiding this comment

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

Good progress! I left a lot of comments, but I think it's mostly polishing up your work.

twine/utils.py Outdated Show resolved Hide resolved
tests/test_upload.py Outdated Show resolved Hide resolved
tests/test_upload.py Outdated Show resolved Hide resolved
tests/test_upload.py Show resolved Hide resolved
twine/commands/upload.py Outdated Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
@VikramJayanthi17
Copy link
Contributor Author

I added the changes suggested by @bhrutledge, thanks for the help. I wasn't sure what the appropriate docstring would be for the test_get_file_size function, currently it is """Compare the size of a file to the expected value.""" but I am open to suggestions on what to change it to.

@VikramJayanthi17 VikramJayanthi17 force-pushed the testing-add-uploaded-packages-to-verbose branch from b745b13 to 3954ad9 Compare June 10, 2020 00:32
Copy link
Contributor

@bhrutledge bhrutledge left a comment

Choose a reason for hiding this comment

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

Nice! I think this is functionally complete. I'm nit-picking the tests and code style, since we've spent a lot of time on that recently, and I think there are some useful tools and patterns to keep in mind.

twine/utils.py Outdated Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
tests/test_upload.py Outdated Show resolved Hide resolved
tests/test_upload.py Outdated Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
@VikramJayanthi17
Copy link
Contributor Author

I made the changes requested by @bhrutledge, thanks for the feedback.

tests/test_utils.py Outdated Show resolved Hide resolved
tests/test_upload.py Outdated Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
@VikramJayanthi17
Copy link
Contributor Author

Added the changes requested by @bhrutledge : eb70d69

The changes that need to be made regarding signatures are still under discussion and I will add any changes to this PR if needed.

Copy link
Contributor

@bhrutledge bhrutledge left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes!

@di di removed the request for review from callmecampos June 11, 2020 19:03
@VikramJayanthi17
Copy link
Contributor Author

As discussed above, I changed the upload function by breaking apart the logic of signing and uploading packages. We now print the signature files of the distributions being uploaded. The print statement happens after the PackageFile instantiating loop and before the uploads are attempted or the skip messages are printed. This means that we are signing packages that may be skipped as mentioned by @bhrutledge.
I believe this location for printing would provide the most information to the user(which distributions and signatures we are attempting to upload) without having other messages(like skips) or errors(while uploading) output at the same time.
Screen Shot 2020-06-11 at 12 46 56 PM

@bhrutledge bhrutledge self-requested a review June 11, 2020 21:38
file_size = utils.get_file_size(filename)
print(f" {filename} ({file_size})")
print(f" {package.signed_filename} (Signature)")
Copy link
Member

Choose a reason for hiding this comment

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

Not ever package will have a signed filename. What happens here if it doesn't have a signature file? What does this print? Folks generating signatures should also know that a .asc file is the signature, what are we hoping to communicate by putting (Signature) at the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, thanks for the feedback. Would you suggest printing the filename like the other files(without anything to indicate that its a signature)? I will also make this print conditional on upload_settings.sign or if its a pre-signed distribution.

Copy link
Contributor

@bhrutledge bhrutledge left a comment

Choose a reason for hiding this comment

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

Nice work. Almost there, I think.

twine/commands/upload.py Outdated Show resolved Hide resolved
twine/commands/upload.py Outdated Show resolved Hide resolved
@VikramJayanthi17
Copy link
Contributor Author

VikramJayanthi17 commented Jun 12, 2020

I added the changes @bhrutledge requested.

Not ever package will have a signed filename. What happens here if it doesn't have a signature file? What does this print?

Additionally I added a clause to check that the package signature will only be displayed if upload_settings.sign is true or if the distribution is pre-signed to address this issue that @sigmavirus24 pointed out. Thanks for the help.

I also added tests to test _make_package and to check that upload prints signatures if appropriate.

@VikramJayanthi17 VikramJayanthi17 force-pushed the testing-add-uploaded-packages-to-verbose branch from 31116c4 to d3d26df Compare June 13, 2020 00:50
@VikramJayanthi17 VikramJayanthi17 force-pushed the testing-add-uploaded-packages-to-verbose branch from 9ec7f3d to 9bcb57e Compare June 15, 2020 21:25
Copy link
Contributor

@bhrutledge bhrutledge left a comment

Choose a reason for hiding this comment

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

Thanks, @VikramJayanthi17. This looks good to me. @sigmavirus24 How about you?

print(f"Uploading distributions to {repository_url}")

packages_to_upload = [
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to allocate a list here? Would it be better to create a generator and use that in the for-loop? I don't think that's a blocker, just food for thought for @VikramJayanthi17

@sigmavirus24 sigmavirus24 merged commit b1c2191 into pypa:master Jun 16, 2020
@bhrutledge
Copy link
Contributor

@VikramJayanthi17 This was released in 3.2.0. Thanks again for your work on this.

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

5 participants