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

support deno lint #162

Merged
merged 10 commits into from
Sep 6, 2020
Merged

support deno lint #162

merged 10 commits into from
Sep 6, 2020

Conversation

axetroy
Copy link
Contributor

@axetroy axetroy commented Aug 29, 2020

close #116

1

Please wait for these two issues to be resolved before merging

@axetroy axetroy changed the title [WIP] add linter [WIP] support deno lint Aug 29, 2020
@lucacasonato
Copy link
Member

lucacasonato commented Sep 2, 2020

@axetroy denoland/deno#7303 has landed in Deno, so all required features should now be available on master. The 1.3.3 release on Friday will contain these features.

@bartlomieju
Copy link
Member

Just a note - line numbers are still 1-based.

@lucacasonato
Copy link
Member

@axetroy 1.3.3 has been released with all required features.

@axetroy
Copy link
Contributor Author

axetroy commented Sep 5, 2020

notable change:

  1. The minimum version number supports v1.3.3 of Deno. A warning will be issued if it is lower than the minor version

  2. To use lint, you need to enable deno.unstable = true and deno.lint = true

1

@axetroy
Copy link
Contributor Author

axetroy commented Sep 5, 2020

/cc @lucacasonato @bartlomieju @ry it's ready for review

Since my mother tongue is not English, So if some English descriptions are not accurate, please correct me.

Two quick fixes have been added currently

  1. // deno-lint-ignore-next-line [rule]
  2. // deno-lint-ignore-file
  3. // deno-lint-ignore [rule] not yet.

@axetroy axetroy changed the title [WIP] support deno lint support deno lint Sep 5, 2020
@bartlomieju
Copy link
Member

  1. // deno-lint-ignore-next-line [rule]

We don't support this directive

  1. // deno-lint-ignore-file
  2. // deno-lint-ignore [rule] not yet.

But we support these two

@axetroy
Copy link
Contributor Author

axetroy commented Sep 5, 2020

  1. // deno-lint-ignore-next-line [rule]

We don't support this directive

  1. // deno-lint-ignore-file
  2. // deno-lint-ignore [rule] not yet.

But we support these two

Is there something wrong? // deno-lint-ignore-next-line [rule] did works in v1.3.3 although I did not see the information of deno help lint

@bartlomieju
Copy link
Member

  1. // deno-lint-ignore-next-line [rule]

We don't support this directive

  1. // deno-lint-ignore-file
  2. // deno-lint-ignore [rule] not yet.

But we support these two

Is there something wrong? // deno-lint-ignore-next-line [rule] did works in v1.3.3 although I did not see the information of deno help lint

Then it seems to be a bug in implementation... 😢 currently deno lint uses following directives:

fn create_linter(syntax: Syntax, rules: Vec<Box<dyn LintRule>>) -> Linter {
  LinterBuilder::default()
    .ignore_file_directives(vec!["deno-lint-ignore-file"]) <--- ignore whole file
    .ignore_diagnostic_directives(vec![
      "deno-lint-ignore",                             <---- ignore certain diagnostic 
      "eslint-disable-next-line",                 <---- ignore certain diagnostic (compatibility with ESLint)
    ])
    .lint_unused_ignore_directives(true)
    // TODO(bartlomieju): switch to true
    .lint_unknown_rules(false)
    .syntax(syntax)
    .rules(rules)
    .build()
}

I'll try to fix this problem soon, but @axetroy please don't add deno-lint-ignore-next-line - I don't intend to support this directive.

@axetroy
Copy link
Contributor Author

axetroy commented Sep 5, 2020

@bartlomieju updated.

BTW. I think deno-lint-ignore-next-line is necessary.

// deno-lint-ignore [rule] should ignore the rule in the whole file.
// deno-lint-ignore-next-line [rule] only ignore the error in the next line.

@bartlomieju
Copy link
Member

// deno-lint-ignore-next-line [rule] only ignore the error in the next line.

That's what deno-lint-ignore does - it only applies to the next line. I think that -next-line is superfluous.

@axetroy
Copy link
Contributor Author

axetroy commented Sep 5, 2020

// deno-lint-ignore-next-line [rule] only ignore the error in the next line.

That's what deno-lint-ignore does - it only applies to the next line. I think that -next-line is superfluous.

So, what if I want to ignore a type of error in the file? To ignore all of them, I have to add // deno-lint-ignore [rule] in every line. This is unfriendly for development.

@bartlomieju
Copy link
Member

@axetroy we can add rule codes to deno-lint-ignore-file [codes...]

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

This is looking very promising. There are a few issues I found while testing:

  • the ignore next line rule quickfix does not work. It gives the error command 'deno._ignore_text_line_hint' not found.
  • As the source of the error, Deno Language Server is shown in the popover. It would be nice if this said deno_lint instead. So the error shown would be Empty switch statement deno_lint(no-empty). If this is not possible Empty switch statement Deno(lint/no-empty) would be good too.

@axetroy
Copy link
Contributor Author

axetroy commented Sep 6, 2020

@lucacasonato

the ignore next line rule quickfix does not work. It gives the error command 'deno._ignore_text_line_hint' not found.

This is a typo and have been fixed. 0de60e6

@axetroy
Copy link
Contributor Author

axetroy commented Sep 6, 2020

@lucacasonato
rename Deno Language Server to deno_lint seem to get a problem. wait for a minute and I am trying.

@axetroy
Copy link
Contributor Author

axetroy commented Sep 6, 2020

@lucacasonato rename Deno Language Server to deno_lint done
1

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

Thanks @axetroy! This is a great feature to have - I will create a 2.2 release for this feature today.

@lucacasonato lucacasonato merged commit 63aac9c into denoland:master Sep 6, 2020
@axetroy axetroy deleted the linter branch September 6, 2020 16:28
CGQAQ pushed a commit to CGQAQ/vscode_deno that referenced this pull request Sep 13, 2020
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.

Add Linter support.
3 participants