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

refactor(core): ordered paths, project manifest, tsconfig #3699

Merged
merged 20 commits into from
Aug 27, 2024

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented Aug 22, 2024

Summary

This applies the following changes:

  • creates a new type called 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 called FileKind. We use this bit flag to determine the order of paths to evaluate.
  • Aligned the project concept and the manifest concept. Before, the manifest was not aligned with the concept of the workspace project.
  • Added a first deserialisation of the tsconfig.json inside the NodeJsProject type
  • Removed EvaluatedPaths and merged its logic into BiomePath. 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.

@github-actions github-actions bot added A-CLI Area: CLI A-Core Area: core A-Project Area: project A-LSP Area: language server protocol labels Aug 22, 2024
Copy link

codspeed-hq bot commented Aug 22, 2024

CodSpeed Performance Report

Merging #3699 will not alter performance

Comparing feat/multi-files (f3fc6c4) with main (61cedcf)

Summary

✅ 101 untouched benchmarks

@ematipico ematipico marked this pull request as ready for review August 26, 2024 12:51
crates/biome_fs/src/path.rs Outdated Show resolved Hide resolved
crates/biome_fs/src/path.rs Outdated Show resolved Hide resolved
crates/biome_fs/src/path.rs Outdated Show resolved Hide resolved
crates/biome_fs/src/path.rs Outdated Show resolved Hide resolved
crates/biome_fs/src/path.rs Outdated Show resolved Hide resolved
crates/biome_fs/src/path.rs Show resolved Hide resolved
crates/biome_fs/src/path.rs Outdated Show resolved Hide resolved
crates/biome_fs/src/path.rs Show resolved Hide resolved
crates/biome_fs/src/path.rs Outdated Show resolved Hide resolved
crates/biome_project/src/node_js_project/tsconfig_json.rs Outdated Show resolved Hide resolved
Comment on lines 100 to 119
/// Adds a file kind to the current file
pub fn with_file_kind(mut self, kind: FileKind) -> Self {
self.kind.0.insert(kind);
self
}
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member Author

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

crates/biome_fs/src/path.rs Outdated Show resolved Hide resolved
Comment on lines +39 to +73
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
};
}
Copy link
Member

@Conaclos Conaclos Aug 26, 2024

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.

Copy link
Member Author

@ematipico ematipico Aug 26, 2024

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

crates/biome_service/src/dome.rs Show resolved Hide resolved
crates/biome_cli/src/commands/lint.rs Outdated Show resolved Hide resolved
@@ -177,14 +178,23 @@ fn traverse_inputs(
}));

let paths = ctx.evaluated_paths();

let dome = Dome::new(paths);
Copy link
Contributor

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 :)

crates/biome_cli/src/execute/traverse.rs Outdated Show resolved Hide resolved
crates/biome_fs/src/path.rs Outdated Show resolved Hide resolved
crates/biome_fs/src/path.rs Outdated Show resolved Hide resolved
crates/biome_service/src/workspace/server.rs Outdated Show resolved Hide resolved
crates/biome_fs/src/path.rs Outdated Show resolved Hide resolved
crates/biome_fs/src/path.rs Show resolved Hide resolved
Copy link
Contributor

@arendjr arendjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

@ematipico ematipico merged commit 88f1d5e into main Aug 27, 2024
10 of 14 checks passed
@ematipico ematipico deleted the feat/multi-files branch August 27, 2024 07:46
@Conaclos Conaclos added the A-Changelog Area: changelog label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-CLI Area: CLI A-Core Area: core A-LSP Area: language server protocol A-Project Area: project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants