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

Merged

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 2 times, most recently 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.

@abravalheri abravalheri force-pushed the _distutils.modified.newer_pairwise_group branch from 56a626c to f1d59cc Compare October 21, 2024 14:00
Avasam and others added 3 commits October 21, 2024 15:38
@abravalheri abravalheri force-pushed the _distutils.modified.newer_pairwise_group branch from f1d59cc to 82aef16 Compare October 21, 2024 14:38
@abravalheri abravalheri force-pushed the _distutils.modified.newer_pairwise_group branch from eb3a95d to 250b622 Compare October 21, 2024 15:02
@Avasam
Copy link
Contributor Author

Avasam commented Oct 21, 2024

@abravalheri btw this can be reduced to a single commit. No need to keep commit history here.

@abravalheri abravalheri merged commit 99c75c9 into pypa:main Oct 21, 2024
23 of 25 checks passed
@abravalheri
Copy link
Contributor

Thank you!

@Avasam Avasam deleted the _distutils.modified.newer_pairwise_group branch October 21, 2024 16:46
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