+
Skip to content
This repository was archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_service): ignore files #3146

Merged
merged 6 commits into from
Sep 14, 2022
Merged

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Sep 1, 2022

Summary

Closes #2827

Configuration

This PR adds support for ignoring files via configuration. The new field has been added to the formatter and the linter sections.

{
	"formatter": {
		"ignore": ["path/to"]
	},
	"linter": {
		"ignore": ["*.ts"]
	}
}
  • the functions deserialize_globals and serialize_globals have been renamed to deserialize_set_of_strings and serialize_set_of_strings, so I could apply the same functions for the fields globals and ignore;

Matcher

I forked the create glob locally. I just copied the engine (parser) of the unix shell style patterns, as we are not interested in actual glob function, which does a traversal of the file system. The only changes that I applied to the crate are merely around clippy - it seems there's low activity on the code - and removed some tests. I won't be able to answer technical questions around this crate.

I added license files and credits to the website.

I created a small wrapper called Matcher to add patterns and run the matcher against paths and strings. It also holds a HashMap to track the already checked paths/strings.

There's a catch with this crate: the crate doesn't support patterns like "test.ts". I tested it myself and I think it's intentional. I added some basic check in the Matcher struct to cover this cases, I figured that users might want to ignore single files.

Workspace

The check is done in two places inside the workspace:

  • inside the function support_features
  • inside a newly created function called is_file_ignored

support_features now returns a new struct which olds an optional reason that explains why the workspace doesn't support a certain file. Before, we were not distinguish the reason why a file couldn't be supported, and this caused some issue that are not fixed. If you check the snapshots, you might notice some deletions of some code that was out of place.

We now use an enum called UnsupportedReason which tells the traversal/LSP why a file is not supported, and based on that they can react accordingly.

is_file_ignored is in the workspace Workspace. This new function is now used in most of all functions that act on some files, I am not sure if I should extend the functionality also on get_syntax_tree and get_control_flow_graph.

The usage of is_file_ignored has been placed before the parsing starts. There's no need to actually parse a file if it's ignored.

When a file is ignored, we intentionally return an error. It's up to the clients to decide to ignore the error or deal with it. This is a pattern we already apply in case a file is not supported.

Test Plan

I added two new tests for the CLI.

I manually built the VSCode extension and made sure that I don't see diagnostics if a file is ignored. And also if the file doesn't get formatted.

@netlify
Copy link

netlify bot commented Sep 1, 2022

Deploy Preview for rometools canceled.

Name Link
🔨 Latest commit 4b0bd73
🔍 Latest deploy log https://app.netlify.com/sites/rometools/deploys/6321ac9646649600089a7554

@ematipico ematipico temporarily deployed to aws September 1, 2022 16:01 Inactive
@ematipico ematipico force-pushed the feature/configuration-ignore-files branch from 52efef4 to ae1c8bf Compare September 1, 2022 16:51
@ematipico ematipico temporarily deployed to aws September 1, 2022 16:51 Inactive
@github-actions
Copy link

github-actions bot commented Sep 1, 2022

@ematipico ematipico force-pushed the feature/configuration-ignore-files branch from ae1c8bf to 472efd7 Compare September 1, 2022 16:57
@ematipico ematipico temporarily deployed to aws September 1, 2022 16:57 Inactive
@ematipico ematipico force-pushed the feature/configuration-ignore-files branch from 472efd7 to 3e195e1 Compare September 1, 2022 17:00
@ematipico ematipico temporarily deployed to aws September 1, 2022 17:00 Inactive
@ematipico ematipico force-pushed the feature/configuration-ignore-files branch from 3e195e1 to 2e77ea3 Compare September 2, 2022 07:44
@ematipico ematipico temporarily deployed to aws September 2, 2022 07:45 Inactive
@ematipico ematipico force-pushed the feature/configuration-ignore-files branch from 2e77ea3 to c224f7e Compare September 2, 2022 07:46
@ematipico ematipico temporarily deployed to aws September 2, 2022 07:46 Inactive
@ematipico ematipico force-pushed the feature/configuration-ignore-files branch from c224f7e to ddd3b90 Compare September 2, 2022 07:48
@ematipico ematipico temporarily deployed to aws September 2, 2022 07:48 Inactive
@ematipico ematipico force-pushed the feature/configuration-ignore-files branch from ddd3b90 to 235f844 Compare September 2, 2022 07:49
@ematipico ematipico temporarily deployed to aws September 2, 2022 07:49 Inactive
@ematipico ematipico force-pushed the feature/configuration-ignore-files branch from 235f844 to 0ba8b31 Compare September 2, 2022 10:02
@ematipico ematipico temporarily deployed to aws September 2, 2022 10:02 Inactive
@ematipico ematipico force-pushed the feature/configuration-ignore-files branch from 0ba8b31 to ef73059 Compare September 2, 2022 10:03
@ematipico ematipico temporarily deployed to aws September 2, 2022 10:03 Inactive
@ematipico ematipico force-pushed the feature/configuration-ignore-files branch from ef73059 to 566a4fc Compare September 2, 2022 12:13
@ematipico ematipico temporarily deployed to aws September 2, 2022 12:13 Inactive
@ematipico ematipico added the A-Project Area: project configuration and loading label Sep 2, 2022
@ematipico ematipico force-pushed the feature/configuration-ignore-files branch from 566a4fc to b321ddf Compare September 2, 2022 14:03
@ematipico ematipico temporarily deployed to aws September 2, 2022 14:03 Inactive
@ematipico ematipico force-pushed the feature/configuration-ignore-files branch from b321ddf to 2800be7 Compare September 2, 2022 14:36
@ematipico ematipico temporarily deployed to aws September 2, 2022 14:36 Inactive
@ematipico ematipico added the A-Core Area: core label Sep 2, 2022
@ematipico ematipico added this to the 0.10.0 milestone Sep 2, 2022
Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

How does the traversal of a directory work. Does the implementation use the glob patterns to filter out directories/files before calling into format / pull diagnostics of a single file ?

@ematipico
Copy link
Contributor Author

ematipico commented Sep 5, 2022

How does the traversal of a directory work.

Our traversal works via a trait - among all our traits - called TraversalContext. While traversing the file system and we encounter some files, we firstly check if these files can be handled by Rome. Handled has different meanings: if a file can be parsed, if it's not ignored, if the feature we're executing - formatter, linter, etc. - is enabled.
Before reading a file from disk, we run the function TraversalContext::can_handle. The implementation of the function can_handle interrogates the workspace's function supports_feature and from there we know if a file can be processed. If so, then we read it from the file system.

Does the implementation use the glob patterns to filter out directories/files before calling into format / pull diagnostics of a single file ?

Yes, this is done because we also want support cases where we directly interrogate a Workspace: via LSP or Node.js for example. This means that before "processing a file" - which starts with the parsing phase - we have to check with Rome if it's able to handle that particular file.

@ematipico ematipico force-pushed the feature/configuration-ignore-files branch from 31c4e10 to 38d2c0d Compare September 7, 2022 10:42
@ematipico ematipico temporarily deployed to netlify-playground September 7, 2022 10:42 Inactive
@ematipico ematipico temporarily deployed to netlify-playground September 7, 2022 11:10 Inactive
@ematipico ematipico requested a review from leops September 12, 2022 10:43
@ematipico ematipico force-pushed the feature/configuration-ignore-files branch from 2ed887e to 79d5e69 Compare September 12, 2022 10:43
@ematipico ematipico temporarily deployed to netlify-playground September 12, 2022 10:43 Inactive
@MichaReiser MichaReiser removed their request for review September 12, 2022 11:04
@MichaReiser
Copy link
Contributor

I'll leave it to @leops to review this PR as he has more context.

Would you mind doing some profiling. E.g. compare wall time of formatting/linting rome classic before/after your change?

@ematipico
Copy link
Contributor Author

ematipico commented Sep 13, 2022

Would you mind doing some profiling. E.g. compare wall time of formatting/linting rome classic before/after your change?

Using time, commands ran using the --release version

this PR

format command

The number of diagnostics exceeds the number allowed by Rome.
Diagnostics not shown: 1684.
Compared 3961 files in 1655ms
Skipped 1405 files
Error: errors where emitted while running checks
6.00s user 3.51s system 32% cpu 29.004 total

Here most of the time is spent printing diagnostics

check command

The number of diagnostics exceeds the number allowed by Rome.
Diagnostics not shown: 5097.
Checked 5366 files in 744ms
Error: errors where emitted while running checks
2.31s user 0.42s system 216% cpu 1.263 total

main

format command

The number of diagnostics exceeds the number allowed by Rome.
Diagnostics not shown: 1684.
Compared 3961 files in 1614ms
Skipped 1405 files
Error: errors where emitted while running checks
6.04s user 3.46s system 33% cpu 28.442 total

check command

The number of diagnostics exceeds the number allowed by Rome.
Diagnostics not shown: 5097.
Checked 5366 files in 879ms
Error: errors where emitted while running checks
2.31s user 0.42s system 201% cpu 1.356 total

@ematipico ematipico force-pushed the feature/configuration-ignore-files branch from 79d5e69 to ee1c872 Compare September 13, 2022 10:40
@ematipico ematipico requested a review from xunilrj as a code owner September 13, 2022 10:40
@ematipico ematipico temporarily deployed to netlify-playground September 13, 2022 10:40 Inactive
@ematipico ematipico temporarily deployed to netlify-playground September 14, 2022 10:27 Inactive
@ematipico ematipico requested a review from leops September 14, 2022 10:29
@ematipico ematipico merged commit 8364d1c into main Sep 14, 2022
@ematipico ematipico deleted the feature/configuration-ignore-files branch September 14, 2022 14:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Core Area: core A-Project Area: project configuration and loading
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

support for ignore patterns
3 participants
点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载