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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add explanation for ignore directives #1229

Merged
merged 4 commits into from
Jan 23, 2024

Conversation

chengr4
Copy link
Contributor

@chengr4 chengr4 commented Jan 18, 2024

Hi,

This is my first contribute in deno codebase. I feel nervous and hope it can go well.

This PR is for #1076.

Please check my updates and slowly guide me to the right direction 馃檹. Thanks in advance

TODO

  • Add test case for test_parse_global_ignore_directives (done at 2024.01.20)
  • Feature: add multi-line

@CLAassistant
Copy link

CLAassistant commented Jan 18, 2024

CLA assistant check
All committers have signed the CLA.

@bartlomieju
Copy link
Member

Looks like a good start. Please run tools/format.ts to format the code and make CI happy.

	According to "ban-types" riles, `Object` is one of types to ignore
@chengr4
Copy link
Contributor Author

chengr4 commented Jan 20, 2024

hi @bartlomieju,
I fixed CI and added test cases for test_parse_global_ignore_directives. Please check it out when you get time, and tell me anything I missed 馃檹.

BTW, There are two things I have zero confidence.

  1. Variable Naming: eg. comment_text_without_reason and IGNORE_COMMENT_REASON_RE <= I hope these are ok.
let comment_text =
        IGNORE_COMMENT_CODE_RE.replace_all(&comment_text_without_reason, ",");

About adding & in front of comment_text_without_reason, I only followed the instruction of the compiler and hope it was the best practice.

Comment on lines +210 to +211
// deno-lint-ignore no-explicit-any no-empty no-debugger -- reason for ignoring
function foo(): any {}
Copy link
Member

Choose a reason for hiding this comment

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

This looks good, I have only one worry - it will be easy to make comments over 80 or 100 characters with explanation. I wonder if we could figure out a way to make these comment multi-line.

Could you try to add a test case for something like:

// deno-lint-ignore no-explicit-any no-empty no-debugger -- super, duper, very long
// reason for ignoring
function foo(): any {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried the test case you gave. it was able to be passed.
but dlint with real TS file did not work.

Do you want to finish multi-line issue in this PR? Or we can open a new issue for it with new PR.

P.s. A small question: Is it common to add comments under 'lint ignore' (Is it common in Node.js)? I personally have never seen it before.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we can do multi-line support in a follow up. Let's land it as is.

@bartlomieju
Copy link
Member

hi @bartlomieju, I fixed CI and added test cases for test_parse_global_ignore_directives. Please check it out when you get time, and tell me anything I missed 馃檹.

BTW, There are two things I have zero confidence.

  1. Variable Naming: eg. comment_text_without_reason and IGNORE_COMMENT_REASON_RE <= I hope these are ok.
let comment_text =
        IGNORE_COMMENT_CODE_RE.replace_all(&comment_text_without_reason, ",");

About adding & in front of comment_text_without_reason, I only followed the instruction of the compiler and hope it was the best practice.

Yeah, these seems fine 馃憤

@chengr4
Copy link
Contributor Author

chengr4 commented Jan 23, 2024

Sorry, I pressed the button accidentally. I didn't intend to rush you 馃槗.

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, nice work @chengr4!

@bartlomieju bartlomieju merged commit 00c9025 into denoland:main Jan 23, 2024
6 checks passed
@chengr4 chengr4 deleted the ignore-directives-explanation branch January 23, 2024 23:54
chengr4 added a commit to chengr4/deno-docs that referenced this pull request Jan 28, 2024
chengr4 added a commit to chengr4/deno_lint that referenced this pull request Jan 28, 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.

None yet

3 participants