-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
use typed_kwargs
for Dependency
#12057
base: master
Are you sure you want to change the base?
use typed_kwargs
for Dependency
#12057
Conversation
0e8c1f7
to
4c57557
Compare
Looks like this needs a rebase on master |
7de47e7
to
016f303
Compare
Ready for another review? |
Yes please |
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.
This was pretty easy to follow. Just left a few comments. For what it's worth, I ran this over postgres and saw no regressions!
016f303
to
be4a5cc
Compare
I know I didn't get to all of your comments @tristan957, but I think I hit the majority of them |
Works for me. This is gonna be a great addition to the codebase! |
d323faa
to
f724dc7
Compare
…used" This reverts commit 93c11f2.
This also allows simplifying the DependencyFallbacksHolder.set_fallback a little, since we now know for sure we'll have a list of 0, 1, or 2 elements. It also fixes an incorrect FeatureNew, which said that the single element array/string value was since 0.53; it's actually since 0.54
I'm not sure what validation to do where. Which methods a particular dependency allows are dependency specific, but there are a set of valid ones, and a set of totally deprecated ones/new ones added that it would be good to emit featurechecks for.
I've made native required, not optional. This is required to ensure that sub-dependencies have the same machine target as the parent.
So we can get proper since warnings
We do some injection of values in some cases that we don't allow from the command line, this adds them and allows us to get rid of the rest of our type: ignore comments
Which gets us super close to having this all type safe
We have several values that are deprecated or added, that are currently not handled due to needing access to the Environment. This can be easily fixed by handling them in the interpreter
Since type checkers now view MisingCompiler as a Compiler, and it provides a method to determine that it is missing (language == 'missing'), we don't need to expose it anywhere, it's just an implementation detail of the dependencies.base module.
46eb3e7
to
bde51f3
Compare
If you rebase this, I can give it one more review and approval. Sorry it fell off my radar. |
This is a rather length series (though I've tried to structure it in such a way that we can land small pieces at a time), that converts the
dependency()
function to use typed_kwargs, and makes some cleanups along the way.I've made a deviation from the way we handle TypedDict elsewhere, and that is that I've used the
total=False
argument. What I found when I tried not doing that, is that there is such a tight coupling betweenbuild.Dependency()
and the Interpreter layer is that any internal Dependency instantiation became very verbose. A followup that might be able to solve this is to to switch Dependency to use all named arguments, such that reasonable defaults are always available. As such we still have to use a great deal ofkwargs.get()
, but it does allow for better type checking.I've also converted the dependency cache to use a distinct class for the key, as I found the old approach to give different results when a keyword argument was undefined vs defined to it's default value, which shouldn't be true.
This also gets the
interpreter/dependencyfallbacks.py
module very close to being type safe.I've opened this as a draft because I'm sure there are still failing tests, but cursory reviews are still welcome.