-
-
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
Ensured all methods in setuptools.modified
raise a consistant distutils.error.DistutilsError
type
#4567
Ensured all methods in setuptools.modified
raise a consistant distutils.error.DistutilsError
type
#4567
Conversation
2001168
to
322e6bb
Compare
a7a791d
to
d6db114
Compare
d6db114
to
07b26e7
Compare
a08af03
to
439ec5a
Compare
ece7519
to
5759d67
Compare
newsfragments/4567.bugfix.rst
Outdated
@@ -0,0 +1 @@ | |||
Ensured all methods in ``setuptools.modified`` raise a consistant ``distutils.errors.DistutilsError`` type -- by :user:`Avasam` |
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.
Ensured all methods in ``setuptools.modified`` raise a consistant ``distutils.errors.DistutilsError`` type -- by :user:`Avasam` | |
Ensured methods in ``setuptools.modified`` preferably raise a consistent | |
``distutils.errors.DistutilsError`` type | |
(except in the deprecated use case of ``SETUPTOOLS_USE_DISTUTILS=stdlib``) | |
-- by :user:`Avasam` |
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.
In the case of the exception (e.g.
SETUPTOOLS_USE_DISTUTILS=stdlib
) these methods would raisesetuptools._distutils.error.DistutilsError
error, right?So maybe we have to change the news fragment a bit...
hmm, looking at my tests I think the intention was to be truly the same as distutils.errors.DistutilsError
at all time, but I see how that won't be the case, unless we start wrapping _distutils
methods here to catch & rethrow (let's not do that).
I think that's still an improvement for the "intended usage" (ie: not using deprecated SETUPTOOLS_USE_DISTUTILS=stdlib
). But I'll need to update my tests to reflect that.
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.
I think it is OK to say: "If you need to catch that error, please don't use the deprecated SETUPTOOLS_USE_DISTUTILS=stdlib settings", so I am good with this.
56a626c
to
f1d59cc
Compare
…utils.error.DistutilsError` type
Co-authored-by: Anderson Bravalheri <andersonbravalheri+github@gmail.com>
f1d59cc
to
82aef16
Compare
eb3a95d
to
250b622
Compare
@abravalheri btw this can be reduced to a single commit. No need to keep commit history here. |
Thank you! |
Summary of changes
The following code should now work as expected in all cases:
I'm not entirely certain my test is correct. Hopefully it should be.
Pull Request Checklist
newsfragments/
.(See documentation for details)