-
-
Notifications
You must be signed in to change notification settings - Fork 82
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
Add Distribution.from_name arg validation #389
Conversation
For some reason, CI is failing me. The error is about 'setuptools_scm' not being able to find a version, but this code hasn't changed much from main, where the tests are passing. Is it the branch name maybe? |
In #390, I made another change in a branch/PR and it passed, so there must be something about this change that's affecting the build. |
@@ -548,7 +550,8 @@ def locate_file(self, path): | |||
""" | |||
|
|||
@classmethod | |||
def from_name(cls, name): | |||
@validate_arguments | |||
def from_name(cls, name: Annotated[str, Field(min_length=1)]): |
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.
Why are we introducing a dependency here? Won't we have to port this to CPython?
Also, personally, I'm unconvinced in introducing a dependency for such a simple check 😅
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.
Over a year ago, I presented at the Python Language Summit a proposal to use annotations to provide validators and transformers. The recommendation then was for me to explore pydantic, so I'm doing that now. I'm not certain I want to add a dependency, but I also want to explore the space. If it works well, I'll want to expand its usage to other similar patterns. As for porting to CPython, it's my understanding that parts of pydantic are being ported to CPython. I'm not sure if these parts are or not, but if not, this and related use-cases can help inform a rationale why they should.
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's something particularly elegant about this approach. In addition to not requiring explicit code in the body to validate the parameter, it also offloads from the author the concern about the correct error to raise here. Instead both the author and the consumer can assume that the error will be consistent with similar parameter validations. It also opens up options for optimizing performance by validating checks statically and disabling validations at runtime.
I still need to explore the viability of adding a dependency, especially in light of CPython integration.
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.
As much as I'd like to explore the use of pydantic, I'm now realizing that the cost of adding a dependency here is very high, not only because of the CPython integration but also because of the race condition with setuptools_scm. I'm going to abandon this approach in favor of a manual check in #391. I would eventually like to explore this concept again, but today is not the day.
f905645
to
71cec30
Compare
I think I know the issue. Because this change introduces a dependency, and because setuptools_scm is relying on importlib_metadata, and because the local code with the dependency is present when running |
2626ba2
to
fdbcb79
Compare
I observe this change introduces some performance costs:
Adds between 21 and 63µs to discovery and distribution resolution (both cached and uncached). That seems acceptable. |
cd8d0b8
to
fdbcb79
Compare
Fixes python/cpython#93259