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

no-warning-comments should include the comment itself in the report (UX only) #12327

Closed
fregante opened this issue Sep 27, 2019 · 13 comments · Fixed by #13522
Closed

no-warning-comments should include the comment itself in the report (UX only) #12327

fregante opened this issue Sep 27, 2019 · 13 comments · Fixed by #13522
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules

Comments

@fregante
Copy link
Contributor

fregante commented Sep 27, 2019

I think the point of this rule is to keep track of TODO comments, however this is hard to follow; I'd have to open each file separately to see if I can handle a task.

  source/globals.d.ts:1:11:1   Unexpected todo comment.                                                                   no-warning-comments2:1   Unexpected todo comment.                                                                   no-warning-comments

  source/libs/post-form.ts:9:39:3   Unexpected todo comment.                                                                   no-warning-comments

  source/features/hidden-review-comments-indicator.tsx:37:237:2   Unexpected todo comment.                                                                   no-warning-comments

  source/features/revert-file.tsx:89:489:4   Unexpected todo comment.                                                                   no-warning-comments

What rule do you want to change?

no-warning-comments

Does this change cause the rule to produce more or fewer warnings?

Unchanged. It's only about UX

How will the change be implemented? (New option, new default behavior, etc.)?

New default

What does the rule currently do for this code?

Shown above

What will the rule do after it's changed?

  source/globals.d.ts:1:11:1   FIXME: Drop ignore after #9.                                                                  no-warning-comments2:1   FIXME: Find this info on the page.                                                                   no-warning-comments

  source/libs/post-form.ts:9:39:3   TODO: Improve JSX types for event listeners so we can use... [clipped]                                                                   no-warning-comments

  source/features/hidden-review-comments-indicator.tsx:37:237:2   TODO: Drop some definitions when their related bugs are resolved                                                                   no-warning-comments

  source/features/revert-file.tsx:89:489:4   TODO: Drop `as` after https://github.com/microsoft/TSJS-lib-generator/issues/741                                                                   no-warning-comments
@fregante fregante added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules triage An ESLint team member will look at this issue soon labels Sep 27, 2019
@g-plane g-plane added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Sep 27, 2019
@g-plane
Copy link
Member

g-plane commented Sep 27, 2019

I'll champion this.

@g-plane g-plane self-assigned this Sep 27, 2019
@g-plane
Copy link
Member

g-plane commented Sep 28, 2019

We need at least 3 upvotes from @eslint/eslint-team to accept this enhancement.

@kaicataldo
Copy link
Member

In general, I'm in support of this, though I do think we want to have some kind of prepended message so it's clear what is being warned about.

@platinumazure
Copy link
Member

Should we truncate the comment in case it's really long?

Should we show only the first line in a multiple-line block comment?

@anikethsaha
Copy link
Member

I think we can set a number of char to show in the message report followed by ... .

I think 30-40 chars limit can be good.

@nzakas
Copy link
Member

nzakas commented Jul 14, 2020

@g-plane as the champion, it's up to you to get the support of others on the ESLint team. It looks like we are still missing two more 👍s.

@g-plane
Copy link
Member

g-plane commented Jul 14, 2020

@eslint/eslint-team Would we like to support this?

@anikethsaha
Copy link
Member

anikethsaha commented Jul 20, 2020

+1 for this.

I can think of something like this

  source/globals.d.ts:1:1
  ⚠    1:1   Unexpected todo comment: TODO: something short                                                     no-warning-comments
  ⚠    2:1   Unexpected todo comment: TODO: something really long...                                            no-warning-comments

I can take this if this gets accepted 👍

@mdjermanovic
Copy link
Member

There are 3 👍 from team members now, so marking as accepted.

@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jul 20, 2020
@anikethsaha
Copy link
Member

@g-plane are you working in this?

@g-plane
Copy link
Member

g-plane commented Jul 20, 2020

Currently, no.

@anikethsaha
Copy link
Member

I can work on this if it's ok?

@g-plane
Copy link
Member

g-plane commented Jul 21, 2020

Sure. Feel free to do it.

kaicataldo pushed a commit that referenced this issue Aug 29, 2020
…13522)

* Update: added the comment in no-warning-comments (fixes #12327)

* Update: changed display logic

* Update: changed the char limit and early break

* Chore: refactor the logic

* Chore: smallchanges

* Chore: refactor small

* Chore: added tests and refactor

* Chore:    correct tests
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Mar 18, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Mar 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants