-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Log warnings when skipping editable installations #12779
Conversation
|
5c0b754
to
bee37db
Compare
let search_path = SearchPath::extra(system, path.clone())?; | ||
files.try_add_root( | ||
db.upcast(), | ||
search_path.as_system_path().unwrap(), |
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.
We need to get the path from search_path
because it is the canonicalized path (whereas path
isn't).
This is covered by the symlink test
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.
All the added .as_system_path().unwrap()
calls are kind of a shame though. Could we do the files.try_add_root()
call inside the SearchPath::extra()
call (as soon as we've canonicalised the system path, but before we've wrapped it in the enum)?
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.
A side effect like adding a path to files.try_add_root
feels off to me in the SearchPath
constructor.
But agree, it's a bit an annoyance. The only other solution I can think of is to separate the validation of a system-search path from its construction. But that feels more involved
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.
A side effect like adding a path to
files.try_add_root
feels off to me in theSearchPath
constructor.
To me it feels correct, actually. The constructor creates a validated, normalized search path; once a SearchPath
instance has been created, that's meant to represent that it's been fully validated and can be used without issue by other modules. But one of the necessary steps to get to that point is to register the search path as a FileRoot
; so to me it makes sense to have it as part of the constructor
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.
I still think it couples too many different things together. A search path should be allowed to exist without having a FileRoot
. They don't strictly belong together. It's only site packages where we need one, but not because the site-packages search path would be incomplete without one.
I think my preferred solution would be to actually decouple the existence of a SearchPath
from its validation but that's a longer discussion that we probably don't agree with.
I would prefer to keep it as is for now
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.
fair enough
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.
I can address your concern of too many unwraps by moving the canonicalize call out of the constructor. Would you prefer this?
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.
I can address your concern of too many unwraps by moving the canonicalize call out of the constructor. Would you prefer this?
Yeah... I think so... but we should then add a note to the docs of the constructor that say that it's expected that the path has already been canonicalised before it's passed to the constructor
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.
Okay. I'll do that but I think I'm going to do it as a follow up PR so that I have fewer merge conflicts.
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.
sure
crates/red_knot_python_semantic/src/module_resolver/resolver.rs
Outdated
Show resolved
Hide resolved
let search_path = SearchPath::extra(system, path.clone())?; | ||
files.try_add_root( | ||
db.upcast(), | ||
search_path.as_system_path().unwrap(), |
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.
All the added .as_system_path().unwrap()
calls are kind of a shame though. Could we do the files.try_add_root()
call inside the SearchPath::extra()
call (as soon as we've canonicalised the system path, but before we've wrapped it in the enum)?
Summary
This PR adds warning messages when skipping editable installations instead of skipping them silently.
Test Plan
Not sure how to test this best without spending too much time on it.