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

Feature: no-inline-comments: Allow exception inside JSX #11270

Closed
Assignees

Comments

@alexilyaev
Copy link

@alexilyaev alexilyaev commented Jan 13, 2019

What rule do you want to change?
no-inline-comments
Does this change cause the rule to produce more or fewer warnings?
Fewer
How will the change be implemented? (New option, new default behavior, etc.)?
Rule options, or change in rule behavior
Please provide some example code that this change will affect:

function App() {
  return (
    <div>
      {/* Some comment */}
      <h1>Some heading</div>
    </div>
  )
}

What does the rule currently do for this code?
Right now no-inline-comments will warn on any comments inside JSX.
What will the rule do after it's changed?
It makes sense to allow comments inside JSX while still disallowing inline comments in regular JS blocks.
Are you willing to submit a pull request to implement this change?
Unfortunately not.

@kaicataldo

This comment has been minimized.

Copy link
Member

@kaicataldo kaicataldo commented Jan 14, 2019

Since comments require the {/} brackets (and I'm assuming that's what triggering the rule to fire), this does seem like a reasonable behavior to add in with an option. I think it would have to ensure that the brackets only contain a comment and treat the brackets as the delimiters to check for tokens around the comment rather than the comment itself.

@kaicataldo kaicataldo added evaluating and removed triage labels Jan 14, 2019
@eslint eslint bot closed this Feb 14, 2019
@eslint eslint bot added the auto closed label Feb 14, 2019
@eslint

This comment has been minimized.

Copy link
Contributor

@eslint eslint bot commented Feb 14, 2019

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@platinumazure

This comment has been minimized.

Copy link
Member

@platinumazure platinumazure commented Feb 14, 2019

Reopening. I'll champion this.

@platinumazure platinumazure reopened this Feb 14, 2019
@platinumazure platinumazure self-assigned this Feb 14, 2019
@g-plane

This comment has been minimized.

Copy link
Member

@g-plane g-plane commented Feb 14, 2019

I'd like to add an option called "ignoreJSX".

@kaicataldo

This comment has been minimized.

Copy link
Member

@kaicataldo kaicataldo commented Apr 25, 2019

@platinumazure @g-plane It sounds like we have two different implementation ideas here (assuming @platinumazure was championing something in neighborhood of what I was suggesting). How should we proceed?

@platinumazure

This comment has been minimized.

Copy link
Member

@platinumazure platinumazure commented Apr 25, 2019

I don't feel strongly either way. I would be okay with an option, but I also note that this rule is pretty much unusable in JSX without the new option and I think we could easily avoid a breaking change in non-JSX code even if we change the default behavior.

@kaicataldo

This comment has been minimized.

Copy link
Member

@kaicataldo kaicataldo commented May 27, 2019

The current behavior in JSX does feel like a bug to me, and I would be okay with changing the behavior by default for JSX as long as other cases aren't affected. If we make this change, I do think we should note the reasoning for this exception in the docs. It would be even better if we could get this into v6!

@kaicataldo

This comment has been minimized.

Copy link
Member

@kaicataldo kaicataldo commented Sep 29, 2019

@eslint/eslint-team Looks like we need one more 👍 or it might be time to close this issue. @platinumazure are you still championing this change?

@jumpconnector

This comment has been minimized.

Copy link

@jumpconnector jumpconnector commented Oct 2, 2019

+1 on this. There should be an option to allow exception on comments inside JSX

@platinumazure

This comment has been minimized.

Copy link
Member

@platinumazure platinumazure commented Oct 4, 2019

@kaicataldo Yes, I'll champion still. I'll poke in the team chat for another 👍 from the team 😄

@mdjermanovic mdjermanovic added accepted and removed evaluating labels Oct 5, 2019
@dorian-marchal

This comment has been minimized.

Copy link

@dorian-marchal dorian-marchal commented Oct 5, 2019

Looks like we need one more 👍

+1 👍

@platinumazure

This comment has been minimized.

Copy link
Member

@platinumazure platinumazure commented Oct 5, 2019

Thanks @dorian-marchal... We're looking for a 👍 from a member of the ESLint team. (Please review our consensus process for more context.)

That said, we did get another team member's 👍, so this issue is accepted.

@mdjermanovic

This comment has been minimized.

Copy link
Member

@mdjermanovic mdjermanovic commented Oct 7, 2019

I'm working on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.