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

lsp: Fix inconsistent processing of ignores #1227

Merged
merged 3 commits into from
Oct 31, 2024

Conversation

charlieegan3
Copy link
Member

@charlieegan3 charlieegan3 commented Oct 30, 2024

In the LS linting, pattern/* would not work when it would in the CLI. This was related to how abs paths were resolved and is fundamentally caused by and issue with URIs being used as file names in the linter.

Now, all files from the LS lint are ignored based on their path with the workspace root URI trimmed.

Screen.Recording.2024-10-30.at.15.45.30.mov

Copy link
Member

@anderseknert anderseknert left a comment

Choose a reason for hiding this comment

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

Fixing this is great! 👏

Passing URIs to a function that at least judging by its name expects a directory.. doesn't feel great. But your hack for dealing with that is much better than mine!

conf := {"rules": {"test": {"rule": {"level": "error"}}}}

not config.excluded_file("test", "rule", "file:///workspace/folder/foo/bar/p.rego") with config.merged_config as conf
}
Copy link
Member

Choose a reason for hiding this comment

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

I did not expect these tests to have to be removed, but perhaps moved to the tests of main. Wr could certainly use some coverage here.

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 think that 72cc35b is a good improvement here. I've brought back some tests, though they are now in main.rego as config is not meant to know about URIs any more.

I have also renamed the rootDir refs to pathPrefix to make it clearer that we're supporting URI or path prefixes with these.

@charlieegan3
Copy link
Member Author

charlieegan3 commented Oct 31, 2024

Passing URIs to a function that at least judging by its name expects a directory

I would actually like to rename the function as it's being used with URIs elsewhere already and is even mentioned in the comment for the function.

I will take another pass at this today.

(Update: done, see #1227 (comment))

test_exclude_files_rule_config_with_path_prefix_relative_name if {
cfg := {"rules": {"testing": {"test": {"level": "error"}}}}

# regal ignore:leaked-internal-reference
Copy link
Member

Choose a reason for hiding this comment

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

I guess you won't be needing this if you rebase from main? :)

In the LS linting, pattern/* would not work when it would in the CLI.
This was related to how abs paths were resolved and is fundamentally
caused by and issue with URIs being used as file names in the linter.

Now, all files from the LS lint are ignored based on their path with the
workspace root URI trimmed.

Signed-off-by: Charlie Egan <charlie@styra.com>
This is more permissive of a URI prefix than rootDir

Signed-off-by: Charlie Egan <charlie@styra.com>
Signed-off-by: Charlie Egan <charlie@styra.com>
@charlieegan3 charlieegan3 force-pushed the ignore-directives-lsp-fix branch from 72cc35b to 1743325 Compare October 31, 2024 16:01
@charlieegan3 charlieegan3 merged commit aeec96b into StyraInc:main Oct 31, 2024
4 checks passed
@charlieegan3 charlieegan3 deleted the ignore-directives-lsp-fix branch October 31, 2024 16:06
charlieegan3 added a commit to charlieegan3/regal that referenced this pull request Jan 6, 2025
* lsp: Fix inconsistent processing of ignores

In the LS linting, pattern/* would not work when it would in the CLI.
This was related to how abs paths were resolved and is fundamentally
caused by and issue with URIs being used as file names in the linter.

Now, all files from the LS lint are ignored based on their path with the
workspace root URI trimmed.

Signed-off-by: Charlie Egan <charlie@styra.com>

* Rename rootDir to path prefix

This is more permissive of a URI prefix than rootDir

Signed-off-by: Charlie Egan <charlie@styra.com>

* Remove ignore:leaked-internal-reference comments

Signed-off-by: Charlie Egan <charlie@styra.com>

---------

Signed-off-by: Charlie Egan <charlie@styra.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants