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

Ensured all methods in setuptools.modified raise a consistant distutils.error.DistutilsError type #4567

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Avasam
Copy link
Contributor

@Avasam Avasam commented Aug 13, 2024

Summary of changes

The following code should now work as expected in all cases:

from setuptools.modified import newer_pairwise_group
from distutils.error import DistutilsError

try:
    newer_pairwise_group(...)
except DistutilsError:
    pass

I'm not entirely certain my test is correct. Hopefully it should be.

Pull Request Checklist

@Avasam Avasam force-pushed the _distutils.modified.newer_pairwise_group branch 4 times, most recently from 2001168 to 322e6bb Compare August 13, 2024 19:06
@Avasam Avasam force-pushed the _distutils.modified.newer_pairwise_group branch 3 times, most recently from a7a791d to d6db114 Compare August 27, 2024 15:31
@abravalheri abravalheri force-pushed the _distutils.modified.newer_pairwise_group branch from d6db114 to 07b26e7 Compare August 28, 2024 10:45
@Avasam Avasam force-pushed the _distutils.modified.newer_pairwise_group branch 4 times, most recently from a08af03 to 439ec5a Compare September 18, 2024 23:17
@Avasam Avasam force-pushed the _distutils.modified.newer_pairwise_group branch from 439ec5a to ece7519 Compare October 15, 2024 16:12
@Avasam Avasam force-pushed the _distutils.modified.newer_pairwise_group branch from ece7519 to 5759d67 Compare October 15, 2024 16:29
setuptools/modified.py Show resolved Hide resolved
@@ -0,0 +1 @@
Ensured all methods in ``setuptools.modified`` raise a consistant ``distutils.errors.DistutilsError`` type -- by :user:`Avasam`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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`

Copy link
Contributor Author

@Avasam Avasam Oct 16, 2024

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 raise setuptools._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.

Copy link
Contributor

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.

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.

2 participants