A bit "cosmetic" changes though needed for implementing more advanced stuff. #2037
Conversation
KOLANICH
force-pushed
the
distr_opts_improvements_cosmetic
branch
2 times, most recently
from
dea5b72
to
f5d63e7
Compare
|
Per the request in #2031, I'm re-opening this for selective consideration. |
There was a problem hiding this comment.
The main change here is suppressing errors on loading the entry points for finalizing distribution options. I'd like to explore the problem's root cause in #2030 before settling on this approach as the solution. Also, please be aware of #1994, which was merged, but not into master (until just now), so that will affect the way you reason about the .order attribute.
| @@ -0,0 +1,4 @@ | |||
| A bit "cosmetic" changes though needed for implementing more advanced stuff. | |||
| * Renamed args names params for some static methods and marked them with ''@staticmethod''. | |||
There was a problem hiding this comment.
What's the purpose of this change? What value does it provide? What issue does it address?
There was a problem hiding this comment.
- Warnings in Finalize distribution options various data #2034 about legacy calling convention (the arg is not named
dist) - it turned out that the methods are static ones, but they look like bound ones. So I have marked them as static, to make this explicit, and also to allow tools for working with source code to capture that fact in their code models.
changelog.d/2037.change.rst
Outdated
| @@ -0,0 +1,4 @@ | |||
| A bit "cosmetic" changes though needed for implementing more advanced stuff. | |||
| * Renamed args names params for some static methods and marked them with ''@staticmethod''. | |||
| * Moved ''0'' into ''default_order_value" variable. | |||
There was a problem hiding this comment.
I'd agree with this change to provide more context. The problem is that the purpose of the default value is already documented in the docstring, so with this change, it's now documented redundantly, opening up the opportunity for the two definitions to diverge. Especially if the code for the function remains small (< 10 lines), I find it unnecessary to make the code self-documenting here.
changelog.d/2037.change.rst
Outdated
| A bit "cosmetic" changes though needed for implementing more advanced stuff. | ||
| * Renamed args names params for some static methods and marked them with ''@staticmethod''. | ||
| * Moved ''0'' into ''default_order_value" variable. | ||
| * When an entry point now fails to be loaded, it doesn't disrupt building process. |
There was a problem hiding this comment.
I can see value in failing rather than suppressing errors. One should be cautious about suppressing errors, as it might be the case that the failed operation might be critical. The warning is helpful, and given the 'fail on warning' behavior of the setuptools test suite, this warning would prevent the core functionality from being suppressed (at least for the duration of the test suite). I think this change encapsulates the main purpose of this PR and probably addresses the underlying bug (which unfortunately isn't referenced here).
There was a problem hiding this comment.
Yeah, I have added it as one of the ways to deal with #2030 when installing setuptools itself.
setuptools/dist.py
Outdated
| ep.load()(self) | ||
| try: | ||
| f = ep.load() | ||
| except Exception as ex: |
There was a problem hiding this comment.
This branch probably needs a test. And it probably deserves to be its own function with a docstring describing its purpose.
There was a problem hiding this comment.
I am not sure how to properly test this. If we move that into a separate function and test only it, it will turn out we are actually testing EntryPoint.load. To test it properly I guess we need to mock iter_entry_points into a one returning our not really existing entry point that always fails by design.
KOLANICH
force-pushed
the
distr_opts_improvements_cosmetic
branch
from
f5d63e7
to
bc0d098
Compare
Also when an entry point now fails to be loaded, it doesn't disrupt building process.
Summary of changes
Closes
Pull Request Checklist