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

fix: Make dlint report syntax/parsing errors #1285

Merged
merged 4 commits into from
Jul 8, 2024

Conversation

HasanAlrimawi
Copy link
Contributor

@HasanAlrimawi HasanAlrimawi commented Jun 12, 2024

Supported dlint reporting of syntax/parsing errors.

The commit is made to address issue 23437 from deno repo

Code changes

Added check for ParsedSource.diagnostics() and when it's not empty, an object of ParseDiagnosticsError holding the vector of ParseDiagnostic objects is returned as an Err object within fn run_linter() in deno_lint/examples/dlint/main.rs file.

PS: The change within this function was preffered over the suggested one in the issue discussion to maintain the types of returned Err in order to keep the same successful flow, since the run_linter function returns AnyError not some specific one like parse_program function, and also it's much simpler (doesn't need to append the ParseDiagnostic single object that returned from deno_ast::parse_program into a vector before returning it as an error).

@lucacasonato

@CLAassistant
Copy link

CLAassistant commented Jun 12, 2024

CLA assistant check
All committers have signed the CLA.

@HasanAlrimawi
Copy link
Contributor Author

Hello @lucacasonato ,
All checks have passed, it would be great If you make a review in order to merge the changes.

@magurotuna
Copy link
Member

@HasanAlrimawi Thanks for working on it! This looks very good, however this fix only applies to dlint which is a standalone binary we can run via cargo run --example dlint. So this won't change any behavior we get when running deno lint command.
I think it's totally fine to get this fix merged on main with the current code, but I also think that you may want to modify more code so that the deno lint command can report syntax errors. Would you like to do this?

…urning them within Err. This change allows lint diagnostics errors to be shown in the same shot as the parse diagnostic errors.
@HasanAlrimawi
Copy link
Contributor Author

HasanAlrimawi commented Jun 25, 2024

@magurotuna Thank you for the informative feedback.

  • I made a change to display the parse diagnostic errors (syntax errors) along with the Lint diagnostic errors at the same time rather than just showing the parse diagnostic errors alone when returning them as an Err. In deno-lint repo

  • I also made a PR to address the same issue in deno Repo.

It would be great if you could review the changes

@HasanAlrimawi
Copy link
Contributor Author

Hello @magurotuna,
Just a kind reminder that the changes from this PR can be merged to deno_lint if you deem it appropriate.

@magurotuna
Copy link
Member

@HasanAlrimawi Sorry for the delay. The approach looks great, but I think it would be even nicer if we could display syntax errors in the same way as lint errors. For instance, the target file's content is let's say:

let a = 'fo

and when we run dlint against this file, the result is displayed as follows:

dlint output

So as shown in this screenshot, syntax errors are displayed with no color while lint errors are colored. It'd be awesome to add color to syntax errors too. (I guess what you need to do to achieve it is using deno_ast::diagnostics::Diagnostic::display of ParsedSource)

That said, given that the purpose of dlint stand-alone executable is mostly to demonstrate the linter's capability independently, displaying syntax errors in a different way than lint errors is acceptable in my opinion. So if you are interested on dealing with it, please go ahead. But otherwise I will just approve and land it as is. Which would you like?

@HasanAlrimawi
Copy link
Contributor Author

Hello @magurotuna ,
I updated the code to show parsing errors using deno_ast::diagnostics::Diagnostic::display.
If everything looks good, I believe it can be merged then.

Copy link
Member

@magurotuna magurotuna left a comment

Choose a reason for hiding this comment

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

Thank you so much, this looks great!

lint output

@magurotuna magurotuna merged commit 06bd13d into denoland:main Jul 8, 2024
6 checks passed
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.

3 participants