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.

Sign up for GitHub

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

Jump to bottom

Tweak upload --verbose suggestion #817

Merged
merged 2 commits into from Oct 8, 2021
Merged

Tweak upload --verbose suggestion #817

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
63d2b6c
Tweak upload --verbose suggestion
bhrutledge Oct 7, 2021
f75b0a2
Create 817.bugfix.rst
bhrutledge Oct 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 1 addition & 6 deletions tests/test_utils.py
View file
Open in desktop
Expand Up @@ -285,12 +285,7 @@ def test_check_status_code_for_missing_status_code(
if verbose:
assert captured.out.count("Content received from server:\nForbidden\n") == 1
else:
assert (
captured.out.count(
"For more details, retry the upload with the --verbose option.\n"
)
== 1
)
assert captured.out.count("--verbose option") == 1


@pytest.mark.parametrize(
Expand Down
11 changes: 7 additions & 4 deletions twine/utils.py
View file
Open in desktop
Expand Up @@ -197,12 +197,15 @@ def check_status_code(response: requests.Response, verbose: bool) -> None:
try:
response.raise_for_status()
except requests.HTTPError as err:
if not verbose:
logger.warning(
"Error during upload. "
"Retry with the --verbose option for more details."
Copy link
Member

Choose a reason for hiding this comment

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

One thought I just had:

We have access to the CLI args, right? Could we print out the command with --verbose injected so they can copy paste it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neat idea. I'll take a look.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking something as simple as:

>>> import itertools
>>> argv = ["twine", "upload", "--skip-progress-bar", "dist/*"]
>>> ' '.join(itertools.chain(argv[:2], ["--verbose"], argv[2:]))
'twine upload --verbose --skip-progress-bar dist/*'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it's not that simple. Running twine upload -r testpypi dist/* with pdb gives me:

(Pdb) sys.argv
['/Users/bhrutledge/Dev/twine/venv/bin/twine', 'upload', '-r', 'testpypi', 'dist/twine-3.4.2.dev12+g6694f57.d20210619-py3-none-any.whl', 'dist/twine-3.4.2.dev12+g6694f57.d20210619.tar.gz']

I think showing that to users will be confusing, so I'm going to merge this as-is.

Copy link
Member

Choose a reason for hiding this comment

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

True, we will only see dist/* if run from tox or Windows. I don't think users will generally be confused about globs being expanded by their shell as that's how it works

Copy link
Contributor Author

@bhrutledge bhrutledge Oct 8, 2021 •
edited

Choose a reason for hiding this comment

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

I'm not sure all/most new users understand shell globbing, and dist/* could potentially expand to lots of files. That, combined with the absolute path of twine, makes for a long string to copy and paste, and also obscures the addition of --verbose.

)

if response.text:
logger.info("Content received from server:\n{}".format(response.text))
if not verbose:
logger.warning(
"For more details, retry the upload with the --verbose option."
)

raise err


Expand Down