-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Raise TypeError
in easy_install.CommandSpec.from_param
#4548
Conversation
TypeError
in easy_install.CommandSpec.from_param
def test_from_param_raises_expected_error(self) -> None: | ||
""" | ||
from_param should raise its own TypeError when the argument's type is unsupported | ||
""" | ||
try: | ||
ei.CommandSpec.from_param(object()) # type: ignore[arg-type] # We want a type error here | ||
except Exception as error: | ||
assert type(error) is TypeError, error | ||
assert ( | ||
str(error) == "Argument has an unsupported type <class 'object'>" | ||
), error | ||
else: | ||
raise AssertionError( | ||
"from_param did not raise any error for argument of unsupported type" | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/pypa/setuptools/actions/runs/10307637126/job/28533318670?pr=4548#step:5:74
setuptools/tests/test_easy_install.py (85.7%): Missing lines 1347
Darn, coverage check includes the test themselves. Is there a better way to do a "expect to fail with specific error" test ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it can be a bit overwhelming.
Is there a better way to do a "expect to fail with specific error" test ?
Have you tried pytest.raises
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I found as well. Looks a lot cleaner, thanks.
fab64a4
to
79d5665
Compare
9674f40
to
190462c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WE can merge this once we are preparing a new major release.
Thank you!
Didn't make it to 73 ^^" |
190462c
to
fbc75bc
Compare
Sorry, I was not cutting that release :( Let's just wait two weeks and merge it anyway. |
It seems that there might be a release soon with changes in distutils... So it might be a good idea to merge this now. |
Summary of changes
Follow-up to #4505 (comment)
As mentioned in that comment, let's include this PR in a major update since, although an edge case, isn't backwards compatible.
Changed the type of error raised by
setuptools.command.easy_install.CommandSpec.from_param
on unsupported argument fromAttributeError
toTypeError
.Pull Request Checklist
newsfragments/
.(See documentation for details)