Add helpful error message for incorrect PyPI URL

[adityasaky]

Closes #499.

[bhrutledge]

Thanks for working on this! I've asked for some changes to make the message more accurate, plus a couple stylistic things.

Also, since this is new conditional behavior, it'd be nice to have a test for it. Unfortunately, we don't have an existing test of check_status_code. Even though this code is in twine.utils, I'd be inclined to test it in test_upload.py, since this is upload-specific code. That makes me wonder if check_status_code should be moved to commands/upload.py, similar to skip_upload.

Are you game to continue working on this?

[adityasaky]

Thanks for the quick review!

... since this is new conditional behavior, it'd be nice to have a test for it. Unfortunately, we don't have an existing test of check_status_code. Even though this code is in twine.utils, I'd be inclined to test it in test_upload.py, since this is upload-specific code. That makes me wonder if check_status_code should be moved to commands/upload.py, similar to skip_upload. Are you game to continue working on this?

Yep, I looked to see where check_status_code was tested and didn't see any. I'd be happy to move it to upload.py and add tests for it, though progress may be a bit slow on that front since the week just started.

Edit: I should probably also update this docstring:

def check_status_code(response, verbose):
    """
    Shouldn't happen, thanks to the UploadToDeprecatedPyPIDetected
    exception, but this is in case that breaks and it does.
    """

[adityasaky]

@bhrutledge let me know if this looks okay and if I need to add more tests. :)

[bhrutledge]

Thanks for the quick turnaround! Looking good. I've got a few requests, plus some additional opportunities for testing. If you're game to continue working on it, we'd appreciate it, but we could also finish it up.

[adityasaky]

@bhrutledge I think I've mostly resolved your suggestions. Let me know about the additional test for deprecated PyPI repository URLs. :)

[bhrutledge]

Thanks for continuing to work on this @adityasaky, and thanks for the ping. It might be the weekend before I can review it, but I'm hoping to get to it sooner.

[adityasaky]

No worries, thanks for the update! :)

[adityasaky]

@bhrutledge Sorry for the delay, let me know if this checks all the boxes. :)

[bhrutledge]

Thanks for continuing to work on this, and bearing with my pedantic feedback. I think this is sufficient, and an improvement, so I'm wary of asking you to keep going. You're welcome to, but I'm also happy to make my suggested changes (which are admittedly a bit vague).

[bhrutledge]

@sigmavirus24 Thanks for the feedback. At the risk of hijacking this PR with maintainer-level discussion, I've asked a few questions.

@adityasaky Thanks for your patience. If you're game to make the requested style changes, awesome. Otherwise, I'm happy to do it, if you grant us permission to make changes to your branch.

[adityasaky]

Thanks for the feedback @bhrutledge, @sigmavirus24! I aim to work on the requested changes later this week / early next week. I hope that's okay.

[bhrutledge]

@adityasaky Of course! Thanks again. Let us know if you have any questions.

[adityasaky]

Apologies for the lateness. I believe I've addressed everything except potentially moving check_status_code back to utils. Please let me know if things look okay!

[bhrutledge]

@adityasaky Thanks for coming back to this! Hopefully just a few more quick changes.

[adityasaky]

I think that's everything, @bhrutledge?

[bhrutledge]

Thanks again for sticking with this. Looks good to me, and the diff is definitely easier to read.

@sigmavirus24 What do you think?

[jaraco]

Thanks everybody for the diligent work on this change that's sure to improve the user experience.