-
-
Notifications
You must be signed in to change notification settings - Fork 541
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
refactor(core): ordered paths, project manifest, tsconfig #3699
Conversation
6800503
to
869ed97
Compare
CodSpeed Performance ReportMerging #3699 will not alter performanceComparing Summary
|
crates/biome_fs/src/path.rs
Outdated
/// Adds a file kind to the current file | ||
pub fn with_file_kind(mut self, kind: FileKind) -> Self { | ||
self.kind.0.insert(kind); | ||
self | ||
} |
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.
The file kind is computed in BiomePath::new
. Do we really need this method? I guess it is used to set the file kind of biome.json
or package.json
to ToHandle
when the json formatter/linter are used?
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 is more for "opt-in" files, where we need to tag additional files other than ToHandle
. For example, GraphQL has two type of files: schemas and queries.
Query files require schema files, so schema files need to be inspected (and collect metadata) before analyzing the query files.
This means a user will tell us which files are the schemas, and Biome will mark them as ToInspect
, and we will use that with_file_kind
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.
Thinking about it more, we can't add the kind like this, so I am going to remove the method and wait until we actually need it
pub fn next_config(&mut self) -> Option<&'a BiomePath> { | ||
return if let Some(path) = self.iter.peek() { | ||
if path.is_config() { | ||
self.iter.next() | ||
} else { | ||
None | ||
} | ||
} else { | ||
None | ||
}; | ||
} | ||
|
||
pub fn next_ignore(&mut self) -> Option<&'a BiomePath> { | ||
return if let Some(path) = self.iter.peek() { | ||
if path.is_ignore() { | ||
self.iter.next() | ||
} else { | ||
None | ||
} | ||
} else { | ||
None | ||
}; | ||
} | ||
|
||
pub fn next_manifest(&mut self) -> Option<&'a BiomePath> { | ||
return if let Some(path) = self.iter.peek() { | ||
if path.is_manifest() { | ||
self.iter.next() | ||
} else { | ||
None | ||
} | ||
} else { | ||
None | ||
}; | ||
} |
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 am unsure to get the idea behind these methods.
They are used in biome_cli::execute::traverse.rs
:
fs.traversal(Box::new(|scope: &dyn TraversalScope| {
while let Some(path) = iter.next_config() {
scope.handle(ctx, path.to_path_buf());
}
while let Some(path) = iter.next_manifest() {
scope.handle(ctx, path.to_path_buf());
}
for path in iter {
scope.handle(ctx, path.to_path_buf());
}
}));
Does this mean that they are yield in-order? i.e. first the configs, then the manifest, and finally the others?
If it is the case, then I don't understand how it is possible because FxHashSet
has no order guarantee? BTreeSet could be used instead if the order is required.
Moreover, if they are meant to be yielded in order, then we could use a single next()
and use loop that iterate until a change in the file kind is detected. This could avoid these methods, and we could remove Peekable
that has some cost.
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.
These are just proposals, they are "used" in a sense that they don't break existing test.
The idea is to have this command throw errors instead, e.g., try_next_manifest
, which will throw an error if we don't consume the configuration files first. Since I didn't have enough time, I cut it short, and I will follow up with another PR
These methods will enforce an order on how files should be processed. The workflow I have in mind:
- detect configuration files, and create a "project" for each one of them (very much like we do in the LSP for workspace projects)
- detect manifest files. If they belong to a project, update it, otherwise we store a new project that doesn't have a configuration file
- detect ignore files
- handle and inspect files
- handle files
Something like that
BTreeSet could be used instead if the order is required.
Yeah I wasn't sure of it, I will apply the change
@@ -177,14 +178,23 @@ fn traverse_inputs( | |||
})); | |||
|
|||
let paths = ctx.evaluated_paths(); | |||
|
|||
let dome = Dome::new(paths); |
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 like the creativity of this name :)
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.
Great work!
Co-authored-by: Victorien Elvinger <victorien@elvinger.fr>
Co-authored-by: Arend van Beelen jr. <arendjr@gmail.com>
926b1a4
to
d227fac
Compare
Summary
This applies the following changes:
Dome
(we can name it later; I didn't know what name to give, so I used a bit of fantasy). This type creates an iterator that iterates over the evaluated paths in certain order. I plan to add more functions, event the ones that will error, because the error will be important.BiomePath
now has flag calledFileKind
. We use this bit flag to determine the order of paths to evaluate.tsconfig.json
inside theNodeJsProject
typeEvaluatedPaths
and merged its logic intoBiomePath
. Less code.Test Plan
Some tests should still pass (some around the manifest). Added new tests for the tsconfig serialisation, and I added valid snapshots that print a debug structure of the Rust types.