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

Reword error message in wheel and wininst read #601

Merged
merged 2 commits into from Apr 22, 2020

Conversation

deveshks
Copy link
Contributor

@deveshks deveshks commented Apr 21, 2020

Fixes and closes #598

Note: I will also update https://github.com/pypa/twine/pull/596/files#diff-b1bd28fb732b18d9d7e161b08847044fR78 with the updated error message, after this PR gets merged and I pull master into that PR branch, or vice versa

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.

Looks good, and the I think checks failed due to making changes to files that are missing test coverage. With that in mind, I'm going to wait on merging this until #596 has been merged.

@deveshks
Copy link
Contributor Author

Agreed, but that will only cover wheel.py. I couldn't find any tests for wininst.py , so would we need to create unit tests for it as well to get this merged?

@bhrutledge
Copy link
Contributor

I couldn't find any tests for wininst.py , so would we need to create unit tests for it as well to get this merged?

I don't think that's necessary, unless you want to. 😉

With the merge of #596, I think this should be rebased on master, and the tests updated to reflect the new message.

@deveshks
Copy link
Contributor Author

deveshks commented Apr 21, 2020

With the merge of #596, I think this should be rebased on master, and the tests updated to reflect the new message.

I did that, but then the codecov/patch only passed for wheel.py, and not wininst.py, since the added code in wininst.py isn't covered by any unit tests

Edit: I can try adding unit tests to wininst.py, but unfortunately I don't have access to a windows machine

@bhrutledge
Copy link
Contributor

I can try adding unit tests to wininst.py, but unfortunately I don't have access to a windows machine

I don't think you need one; the code looks OS-agnostic to me. I suspect that a test_wininst.py could start from a copy of test_wheel.py, replacing instances of .whl with .exe.

But, I'm going to merge this as-is.

@bhrutledge bhrutledge merged commit 0291093 into pypa:master Apr 22, 2020
@deveshks deveshks deleted the reword-err-msg-wheel-wininst branch April 22, 2020 09:55
@deveshks
Copy link
Contributor Author

I don't think you need one; the code looks OS-agnostic to me. I suspect that a test_wininst.py could start from a copy of test_wheel.py, replacing instances of .whl with .exe.

But, I'm going to merge this as-is.

The latest checks run after merging it to master also failed on codecov/patch, but If I understand correctly, future merges will only run codecov/patch on the code changes made to those specific commits, so this failure won't have any effect in the longer run?

@bhrutledge
Copy link
Contributor

future merges will only run codecov/patch on the code changes made to those specific commits, so this failure won't have any effect in the longer run?

Correct. So let's get another PR merged. 😉

@bhrutledge
Copy link
Contributor

@deveshks This was released in 3.2.0. Thanks again for your work on this, and all of the testing and typing work.

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.

Vague exception message in twine.wheel.read and twine.wininst
2 participants