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

feat(lint): Add checked files to JsonLintReporter #26936

Merged

Conversation

skeithtan
Copy link
Contributor

Fixes #26930

@CLAassistant
Copy link

CLAassistant commented Nov 19, 2024

CLA assistant check
All committers have signed the CLA.

@skeithtan skeithtan force-pushed the add-checked-files-to-jsonlintreporter branch from 3ac7ae4 to 91f6ec5 Compare November 19, 2024 18:34
@skeithtan skeithtan marked this pull request as ready for review November 19, 2024 18:37
@skeithtan skeithtan force-pushed the add-checked-files-to-jsonlintreporter branch 5 times, most recently from bd4b10b to 7ac14bf Compare November 19, 2024 22:07
@skeithtan skeithtan force-pushed the add-checked-files-to-jsonlintreporter branch 2 times, most recently from 45431b0 to 02767b3 Compare November 20, 2024 11:31
@skeithtan skeithtan force-pushed the add-checked-files-to-jsonlintreporter branch from 02767b3 to 57d952c Compare November 20, 2024 11:31
@skeithtan skeithtan force-pushed the add-checked-files-to-jsonlintreporter branch from 57d952c to 25cf21d Compare November 20, 2024 12:10
Comment on lines 215 to 216
if !self.checked_files.contains(&d.specifier.to_string()) {
self.checked_files.push(d.specifier.to_string());
Copy link
Member

Choose a reason for hiding this comment

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

To have the consistent format this should probably do d.specifier.to_file_path()

@@ -175,6 +175,7 @@ struct JsonLintReporter {
version: u8,
diagnostics: Vec<JsonLintDiagnostic>,
errors: Vec<LintError>,
checked_files: Vec<String>,
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking this should be sorted just like diagnostics, you can do it in close() method

@skeithtan skeithtan force-pushed the add-checked-files-to-jsonlintreporter branch 3 times, most recently from 63fb28c to 305a7bf Compare November 20, 2024 15:16
@skeithtan skeithtan force-pushed the add-checked-files-to-jsonlintreporter branch from 305a7bf to b8d6cf5 Compare November 20, 2024 15:41
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@bartlomieju bartlomieju merged commit 8f72798 into denoland:main Nov 20, 2024
17 checks passed
bartlomieju pushed a commit to bartlomieju/deno that referenced this pull request Nov 20, 2024
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.

deno lint shows information that deno lint --json does not
3 participants