Make --skip-existing more robust#510
Conversation
This comment has been minimized.
This comment has been minimized.
|
I'm surprised by the codecov failure; will investigate. |
|
I think this is all set. I made the checks more flexible and readable, fixed the missing coverage due to an incorrect test, and took the opportunity to parametrize tests. |
Co-Authored-By: Dustin Ingram <[email protected]>
|
@pypa/twine-maintainers Is it okay if I merge this? I believe it'd be my first merge of a code change, and even though "squash and merge" would be easy to undo, I thought it'd be better to ask permission than forgiveness. 😉 |
| response.reason.startswith(msg_400)) or | ||
| (response.status_code == 403 and msg_403 in response.text))) | ||
| # 403 or 409 status code. | ||
| return any([ |
There was a problem hiding this comment.
So in this case, we generate a list before we can call any which can otherwise be lazy. What if, we choose a lazier way to implement this given tat in with strings can be slow and people expect their CLIs to be quick. (Especially considering text can be a giant HTML page and really really slow to scan through.) In that case, I'd prefer if this looked like:
conditions = [(400, 'already exist', reason), (400, 'updating asset', reason), (403, 'overwrite artifact', text)]
return status == 409 or any(status == status_code and search_for in string for (status_code, search_for, string) in conditions)any will consume the generator and then we won't unnecessarily scan through a potentially giant document every time we see a 403
There was a problem hiding this comment.
Thanks, @sigmavirus24. I'm usually all about laziness; just missed it here. I took a different approach in e366401 (that's actually similar to the prior implementation).
3 participants
Fixes #332.
I realize that checking for strings in the response is still brittle, but given the APIs that we support, I don't see how to avoid it. And, the issue said "less brittle". :)
I didn't spend much time on the tests; just copy-pasted from similar tests. Happy to refactor or tidy up.