Skip to content
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

Merged
merged 3 commits into from
Jul 16, 2024
Merged

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Jul 12, 2024

Summary

This PR adds a ignore::WalkDir like API to System that allows walking a directory tree.

We'll need this to implement the analysis of an entire directory.

Test Plan

Added unit tests.

@MichaReiser MichaReiser added the red-knot Multi-file analysis & type inference label Jul 12, 2024
Copy link

codspeed-hq bot commented Jul 12, 2024

CodSpeed Performance Report

Merging #12297 will not alter performance

Comparing system-walk-directories (7cfe17b) with system-walk-directories (b22ac5e)

Summary

✅ 33 untouched benchmarks

@MichaReiser MichaReiser force-pushed the system-walk-directories branch 2 times, most recently from 2f55fbc to 052432e Compare July 12, 2024 17:52
&self,
path: impl AsRef<SystemPath>,
) -> Result<impl Iterator<Item = Result<DirectoryEntry>> + '_> {
pub fn read_directory(&self, path: impl AsRef<SystemPath>) -> Result<ReadDirectory> {
Copy link
Member Author

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

@MichaReiser MichaReiser marked this pull request as ready for review July 12, 2024 17:59
Copy link
Contributor

github-actions bot commented Jul 12, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

openai/openai-cookbook (error)

warning: Detected debug build without --no-cache.
error: Failed to parse examples/chatgpt/gpt_actions_library/.gpt_action_getting_started.ipynb:11:1:1: Expected an expression
error: Failed to parse examples/chatgpt/gpt_actions_library/gpt_action_bigquery.ipynb:13:1:1: Expected an expression

Formatter (preview)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

openai/openai-cookbook (error)

ruff format --preview

warning: Detected debug build without --no-cache.
error: Failed to parse examples/chatgpt/gpt_actions_library/.gpt_action_getting_started.ipynb:11:1:1: Expected an expression
error: Failed to parse examples/chatgpt/gpt_actions_library/gpt_action_bigquery.ipynb:13:1:1: Expected an expression

@MichaReiser MichaReiser force-pushed the system-walk-directories branch 2 times, most recently from 9f4350a to aedb7fb Compare July 13, 2024 08:43
crates/ruff_db/src/system/walk_directory.rs Outdated Show resolved Hide resolved
crates/ruff_db/src/system/walk_directory.rs Outdated Show resolved Hide resolved
crates/ruff_db/src/system/walk_directory.rs Outdated Show resolved Hide resolved
crates/ruff_db/src/system/walk_directory.rs Outdated Show resolved Hide resolved
crates/ruff_db/src/system/walk_directory.rs Outdated Show resolved Hide resolved
crates/ruff_db/src/system/walk_directory.rs Outdated Show resolved Hide resolved
Comment on lines +242 to +251
Loop {
ancestor: SystemPathBuf,
child: SystemPathBuf,
},

/// An error that occurs when doing I/O
Io {
path: Option<SystemPathBuf>,
err: std::io::Error,
},
Copy link
Contributor

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.

Copy link
Member Author

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.

crates/ruff_db/src/system/test.rs Outdated Show resolved Hide resolved
crates/ruff_db/src/system/os.rs Outdated Show resolved Hide resolved
crates/ruff_db/src/system/os.rs Outdated Show resolved Hide resolved
@MichaReiser MichaReiser enabled auto-merge (squash) July 16, 2024 06:36
@MichaReiser MichaReiser merged commit 85ae02d into main Jul 16, 2024
16 checks passed
@MichaReiser MichaReiser deleted the system-walk-directories branch July 16, 2024 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants