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

New spaced-comment rule to only enforce space after // #2897

Closed
doberkofler opened this issue Jul 1, 2015 · 26 comments
Closed

New spaced-comment rule to only enforce space after // #2897

doberkofler opened this issue Jul 1, 2015 · 26 comments
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

@doberkofler
Copy link
Contributor

Is it no longer possible to only enforce space after // but not after /* ?

@gyandeeps gyandeeps added the triage An ESLint team member will look at this issue soon label Jul 1, 2015
@gyandeeps
Copy link
Member

Can you elaborate by giving an example and stating what do you expect to happen and what is currently happening?

@mathieumg
Copy link
Contributor

I think what @doberkofler means is that spaced-comment will replace spaced-lined-comment which is deprecated (or will be) unless I'm mistaken. With spaced-comment you can't opt-out/opt-in based on the comment type (line or block), can you?

@doberkofler
Copy link
Contributor Author

@mathieumg Exactly!

@gyandeeps
Copy link
Member

we dont have an option like that. right now that rule runs on both (line and block) by default.

@doberkofler
Copy link
Contributor Author

@gyandeeps Having transitioned from jshint/jscs to eslint, I originally wrote the spaced-line-comment rule as we only want to enforce this rule for line comments and jscs allowed me to do this with the requireSpaceAfterLineComment rule. I would no longer be able to enforce this style rule, once the depreciated space-line-comment rule gets removed and I would clearly prefer to either have two separate rules, split the new one into two configurations or keep the old one.

@gyandeeps
Copy link
Member

I think I would support the notion of having that option.
But before we make a decision, we need feedback.
CC @eslint/eslint-team

@nzakas
Copy link
Member

nzakas commented Jul 2, 2015

Let's just add options to specify line:true, block: true, so people can choose.

@nzakas nzakas added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jul 2, 2015
@gyandeeps
Copy link
Member

And by default both are true?

@nzakas
Copy link
Member

nzakas commented Jul 2, 2015

Yes, we need to keep rule defaults the same to avoid breaking people's linting.

@doberkofler
Copy link
Contributor Author

Sounds good to me as this would solve my problem but more generally speaking I think that splitting the rule might be the better option. It seems to me, as if people would use the two comment styles rather differently and maybe they should also be configured independently.

@nzakas
Copy link
Member

nzakas commented Jul 3, 2015

I don't see splitting the rule as an option. We just combined this rule to handle both block and line comments, reverting that change would be unnecessarily disruptive to people.

@mathieumg
Copy link
Contributor

We just combined this rule to handle both block and line comments

Is there any reason behind that merge? space-in-brackets was separated in two different rules for arrays and objects, why would this one be a single rule if there is a case where a user would want different rules for each comment type?

@nzakas
Copy link
Member

nzakas commented Jul 4, 2015

Because the logic is exactly the same. You can always go dig up the commit and issue to see the discussion.

@mathieumg
Copy link
Contributor

I understand the rationale of avoiding code duplication, but if the logic is exactly the same couldn't it be put in a 3rd file which is leveraged by the rule for each comment flavor? Another way would be to have the line and block properties accept configuration rather than just a boolean, but I imagine that would deviate from the [#, {...}] paradigm.

@nzakas
Copy link
Member

nzakas commented Jul 6, 2015

Having a third file becomes a maintenance issue - where do we put file-specific code? Do we end up with "third" files for a lot of rules then? It's much cleaner to have one spot.

In any event, whether we keep one rule or not isn't up for debate here. We can add options to handle the case where line comments should be handled separate from block comments. I think maybe it should be more like:

spaced-comment: [2, {
    line: {
        exceptions: []
    },
    block: {}
}]

@mathieumg
Copy link
Contributor

I understand, regarding the "third file"/file structure.

I think the config example you showed would be good, it only sacrifices the ability to warn/error based on the comment type, and that's not really a problem. At least it allows to enforce different things based on the comment type, as opposed to the true/false approach, and it doesn't duplicate code which was the original intent behind the merge. 👍

@doberkofler
Copy link
Contributor Author

@nzakas Having separate options for line and block command sounds like a good solution.

@xjamundx
Copy link
Contributor

xjamundx commented Aug 5, 2015

I'll throw up a proposed solution to this problem and am happy to work on it as I'm running into similar issues.

@xjamundx
Copy link
Contributor

xjamundx commented Aug 7, 2015

So the suggestion is the following?

Current options:

  • "spaced-comment": [2, "always"]
  • "spaced-comment": [2, "never"]
  • "spaced-comment": [2, "always", { exceptions: [], markers: [] }]

Proposed Options:

  • "spaced-comment": [2, "always"]
  • "spaced-comment": [2, "never"]
  • "spaced-comment": [2, "always", { block: false }]
  • "spaced-comment": [2, "always", { exceptions: [], markers: [] }]
  • "spaced-comment": [2, "always", { exceptions: [], line: false }]
  • "spaced-comment": [2, "always", { block: { exceptions: [] }, line: { markers: [] } }]

The only question for me is what happens in cases like this:

  • "spaced-comment": [2, "always", { exceptions: [], block: { markers: [] } }]

Do the exceptions get applied into the block options? It's starting to get messy....but I guess we could just Object.assign() and shallow merge it...

@doberkofler
Copy link
Contributor Author

@xjamundx I think the option for line and block should rather be completely separated as if it would be two different rules as suggested by @nzakas

spaced-comment: [2, {
    line: {
        exceptions: []
    },
    block: {}
}]

@nzakas
Copy link
Member

nzakas commented Aug 11, 2015

Agreed

@BYK
Copy link
Member

BYK commented Aug 18, 2015

I'm working on this.

BYK added a commit that referenced this issue Aug 24, 2015
@mccord
Copy link

mccord commented Nov 17, 2015

How would you formulate "only apply the rule for line comments, not block" in the current spec? The following does not work because it is expecting an object instead of "false."

"spaced-comment": [2, "always", { block: false }]

@doberkofler
Copy link
Contributor Author

@mccord like this

spaced-comment: [2, {
    line: {
        exceptions: []
    },
    block: {}
}]

@noppa
Copy link

noppa commented Jan 26, 2018

This doesn't seem to work anymore (version 4.16.0). The config @doberkofler suggested fails with error

Configuration for rule "spaced-comment" is invalid:
Value {"line":{"exceptions":[]},"block":{}} should be equal to one of the allowed values.

which I assume means that the "always"/"never" is required as the second item in the list.

With config

"spaced-comment": [2, "always", {
   "line": {
        "exceptions": []
    },
    "block": {}
}]

and code

// Foo
/*Foo*/

gives an error

Expected space or tab after '/*' in comment spaced-comment

Is there still a way to only enforce space after // and ignore multi-line comments completely?

@platinumazure
Copy link
Member

@noppa Please open a new issue and we'll investigate, thanks!

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 7, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 7, 2018
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

No branches or pull requests

9 participants