[ty] Tell the user why we inferred the Python version we inferred #18082
Conversation
AlexWaygood
requested review from
carljm,
sharkdp,
dcreager and
MichaReiser
as code owners
AlexWaygood
added
ty
AlexWaygood
force-pushed
the
alex/tell-me-why-py-version
branch
2 times, most recently
from
0c61c94 to
58e0a85
Compare
|
AlexWaygood
force-pushed
the
alex/tell-me-why-py-version
branch
from
58e0a85 to
e10016c
Compare
|
There was a problem hiding this comment.
This looks great. Unfortunately, I expect this to break persistent caching where we'd use a different persistent cache for different python versions.
For now, I think the best we can do, without breaking persistent caching), is to say if the value comes from the CLI or configuration but without saying exactly from which configuration.
| @@ -239,8 +239,8 @@ impl ProjectMetadata { | |||
| &self.extra_configuration_paths | |||
| } | |||
|
|
|||
| pub fn to_program_settings(&self, system: &dyn System) -> ProgramSettings { | |||
| self.options.to_program_settings(self.root(), system) | |||
| pub fn to_program_settings(&self, db: &dyn crate::Db) -> ProgramSettings { | |||
There was a problem hiding this comment.
I'm sorry, but we can't use db here. It will break our approach to persistent caching.
What's important for persistent caching is that we can resolve the ProgramSettings before constructing the db. We then hash the ProgramSettings and use the hash to lookup the cache file and load the database from it. This allows us to support different persistent caches when running ty with different python versions
| impl<T> RangedValue<T> | ||
| where | ||
| T: Copy, | ||
| { | ||
| pub fn inner_copied(&self) -> T { | ||
| self.value | ||
| } | ||
| } |
There was a problem hiding this comment.
Can't the call site write value = *ranged instead?
| #[returns(ref)] | ||
| pub python_version_source: ValueSource, |
There was a problem hiding this comment.
I'd prefer to wrap PythonVersion in a struct that stores both the source and the version to reduce the top-level fields and make it harder to only update python_version but forget to update the source (which you didn't!)
There was a problem hiding this comment.
Does this also mean a program with the same Python version but defined at separate source would invalidate the entire program? This isn't that important issue as changing the source of a Python version is not going to be that frequent in practice (I think) and the benefit that it provides is more useful IMO.
There was a problem hiding this comment.
We could ignore the source when computing the cache key
| @@ -59,7 +59,7 @@ impl ProjectDatabase { | |||
| // we may want to have a dedicated method for this? | |||
|
|
|||
| // Initialize the `Program` singleton | |||
| let program_settings = project_metadata.to_program_settings(db.system()); | |||
| let program_settings = project_metadata.to_program_settings(&db); | |||
There was a problem hiding this comment.
See the comment above about persistent caching (refer to my other inline comment)
| @@ -17,7 +17,7 @@ pub mod pyproject; | |||
| pub mod settings; | |||
| pub mod value; | |||
|
|
|||
| #[derive(Debug, PartialEq, Eq)] | |||
| #[derive(Debug, PartialEq, Eq, Clone)] | |||
There was a problem hiding this comment.
I'd prefer if this doesn't have to be clone. It's a very heavy data structure. It's not clear to me where we have to clone it.
| #[derive(Debug, PartialEq, Eq, Clone)] | |
| #[derive(Debug, PartialEq, Eq)] |
|
Another way to work around the no db problem is to store the |
Summary
I'm blaming @MichaReiser for this. He said it would be easy in #18068 (comment) and... it wasn't 😆
This PR propagates the origin of the inferred Python version (CLI flag,
pyproject.toml,ty.toml, etc.) from thety_projectcrate to thety_python_semanticcrate. (Specifically, a newpython_version_sourcefield is added to thety_python_semantic::program::Programstruct.) Having the origin of the Python version available allows us to add subdiagnostics that annotate the specific part of the config file that led us to infer a certain Python version.A screenshot to demonstrate this in action:
Test Plan
I added integration tests to the
tycrate.