-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[red-knot] Add walk_directories
to System
#12297
Conversation
CodSpeed Performance ReportMerging #12297 will not alter performanceComparing Summary
|
2f55fbc
to
052432e
Compare
&self, | ||
path: impl AsRef<SystemPath>, | ||
) -> Result<impl Iterator<Item = Result<DirectoryEntry>> + '_> { | ||
pub fn read_directory(&self, path: impl AsRef<SystemPath>) -> Result<ReadDirectory> { |
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'm happy to revert this pack to returning an impl
. I had to change it, because a first version of the walker had to store the current iteration state in a struct
which doesn't work with impl
(Unless you use dyn
). I introduced the named type and thought I'll leave it, because it can be useful.
It's also nice that the type now implements Debug
052432e
to
f35e232
Compare
|
9f4350a
to
aedb7fb
Compare
aedb7fb
to
b22ac5e
Compare
Loop { | ||
ancestor: SystemPathBuf, | ||
child: SystemPathBuf, | ||
}, | ||
|
||
/// An error that occurs when doing I/O | ||
Io { | ||
path: Option<SystemPathBuf>, | ||
err: std::io::Error, | ||
}, |
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.
It seems like most places we prefer having enum variants wrap independent named structs over struct enum variants. I'm curious if you have thoughts on when to choose one vs the other.
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.
To me it depends on how the enum variants are used. Our AST used to use struct enum variants over named structs. This was very cumbersome in downstream tools because it was impossible to pass an entire StmtFunctionDef
, because it lacked a name. That's why you still see many lint rules accepting all individual fields of their node instead of the entire node.
Another motivation for named structs is if you want to lock-down access to individual fields because the enum fields always have the same visibility as the enum itself.
On the other hand, using variant structs allows Rust to be more clever when it comes to layout optimizations and it's a lot less boilerplate to define the enum and to match on it (no nesting).
I'm using struct variants here because
a) I think it's unlikely that some code would want to pass the entire variant (have it a named type) and even if, that code would at most have to accept two arguments
b) The main use case is to match on the variant and extract the fields, which is more brief with struct variants.
Summary
This PR adds a
ignore::WalkDir
like API toSystem
that allows walking a directory tree.We'll need this to implement the analysis of an entire directory.
Test Plan
Added unit tests.