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

`indent` not playing nice with comma-first style #3456

Closed
MarkKahn opened this Issue Aug 20, 2015 · 22 comments

Comments

Projects
None yet
8 participants
@MarkKahn
Copy link

MarkKahn commented Aug 20, 2015

{
    foo : 123
  , bar : 456
}

throws an error with indent expecting 2 spaces instead of 4. However I prefer to align my keys like this instead of either of these:

{
  foo : 123
  , bar : 456
}
{
  foo   : 123
  , bar : 456
}

Is there any way to get indent to be quiet here but keep it on everywhere else? This didn't happen in v0.24, currently on v1.2.0

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Aug 20, 2015

Thanks for the issue! We get a lot of issues, so this message is automatically posted to each one to help you check that you've included all of the information we need to help you.

Reporting a bug? Please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. The source code that caused the problem
  3. The configuration you're using (for the rule or your entire config file)
  4. The actual ESLint output complete with line numbers

Requesting a new rule? Please be sure to include:

  1. The use case for the rule - what is it trying to prevent or flag?
  2. Whether the rule is trying to prevent an error or is purely stylistic
  3. Why you believe this rule is generic enough to be included

Requesting a feature? Please be sure to include:

  1. The problem you want to solve (don't mention the solution)
  2. Your take on the correct solution to problem

Including this information in your issue helps us to triage it and get you a response as quickly as possible.

Thanks!

@eslintbot eslintbot added the triage label Aug 20, 2015

@BYK

This comment has been minimized.

Copy link
Member

BYK commented Aug 20, 2015

@zyklus - can you share your configuration for this rule too? I suspect your indent level is 2 because when I test this with 4 it works.

@BYK

This comment has been minimized.

Copy link
Member

BYK commented Aug 20, 2015

And by "it works" I mean it doesn't give me any warnings.

@MarkKahn

This comment has been minimized.

Copy link
Author

MarkKahn commented Aug 20, 2015

@BYK -- Sorry, yes it's set to [2, 2]

@BYK

This comment has been minimized.

Copy link
Member

BYK commented Aug 20, 2015

@zyklus - then I think it makes sense for the rule to error. I don't think this is something that should be fixed.

@MarkKahn

This comment has been minimized.

Copy link
Author

MarkKahn commented Aug 20, 2015

@BYK -- It's a conflict with a comma-first style and, similar to options like const, allows keys to be properly aligned:

const a = 1,
      b = 2,
      c = 3;

If eslint supports a comma-first style, I believe it should be fixed :) As it is right now, it is impossible to align keys with a comma-first style and this rule enabled.

@BYK

This comment has been minimized.

Copy link
Member

BYK commented Aug 20, 2015

@zyklus - it doesn't conflict with comma-first style actually because it works when you set the indent to 4 with that same code. The issue here is that code not adhering to 2-space indent rules really. If it gave a warning with the following example, I may have considered this as a bug:

var a = {
  some: 1
, name: 2
};

var b = {
  same: 3
 ,like: 4
};
@MarkKahn

This comment has been minimized.

Copy link
Author

MarkKahn commented Aug 20, 2015

I don't consider this "proper" indentation like you're suggesting though:

config = extend({
  params       : args.shift()
, description  : args.shift()
, handler      : args.shift()
, defaultValue : args.shift()
}, config || {});
@MarkKahn

This comment has been minimized.

Copy link
Author

MarkKahn commented Aug 20, 2015

To clarify -- I acknowledge that the code doesn't adhere to a 2-spacing rule, but at the same time I believe it's "properly" spaced given a comma-first style.

So whether or not my initial phrasing of "indent is broken" is correct, I believe there should be some way to make the code pass linting while simultaneously keeping the 2-space rule for all other code.

Consider the purpose of the other options in this rule: to align keys.

// 2, {var:2}
var a,
    b,
    c;

// 2
var a
  , b
  , c;
@BYK

This comment has been minimized.

Copy link
Member

BYK commented Aug 20, 2015

I believe there should be some way to make the code pass linting while simultaneously keeping the 2-space rule for all other code.

Well, you can disable the rule for that part :D

Anyways, I see what you say but still disagree with you. I also think trying to support this would make the already complicated rule even more complicated. That said I'm just one person so let's see what others think. @eslint/eslint-team, please weigh in :)

@MarkKahn

This comment has been minimized.

Copy link
Author

MarkKahn commented Aug 20, 2015

Well, you can disable the rule for that part 💃

how? I don't see a way to disable it specifically for objects. Or are you suggesting wrapping every object block in comments that disable the rule? 😡

@BYK

This comment has been minimized.

Copy link
Member

BYK commented Aug 20, 2015

Or are you suggesting wrapping every object block in comments that disable the rule? 😡

I was suggesting that (and sorry for the dancer emoji, I meant to put a :D there).

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Aug 20, 2015

I agree with @BYK suggestion of disabling the rule here.
@zyklus Here is the link which explains how to disable rules: http://eslint.org/docs/user-guide/configuring#configuring-rules

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Aug 20, 2015

@zyklus While we would love to support this style, it's just not feasible for us to support every indentation style under the sun. Indent is already the most complicated rule that we have, and adding every additional style makes it even more complex. I would also suggest disabling this rule for declarations.

@kentor

This comment has been minimized.

Copy link
Contributor

kentor commented Aug 20, 2015

Is there a way to specify in the indent rule to just turn off checking variable declarations? This has been preventing me from upgrading from 1.0.0, unless I turn off checking indentations entirely. My style is different from that of the OP, and I agree that eslint shouldn't add support for every variable declaration style, but I don't want to add eslint-ignore directives to a lot of existing code.

@MarkKahn

This comment has been minimized.

Copy link
Author

MarkKahn commented Aug 22, 2015

I'm with @kentor on this. Though I don't think my style is that uncommon (there was a chart floating around a while ago that said comma-first was around 20%), I concede that supporting every style is unfeasible. It would be nice to have a way to disable this rule contextually. In my case for object literals. Though yes, it's possible to disable this by surround blocks with comments, that's hideous. If it was a one-off, fine, but adding comments to disable this rule in 100 different places isn't an acceptable "solution".

@BYK

This comment has been minimized.

Copy link
Member

BYK commented Aug 22, 2015

Adding a new option "exceptions" that takes a node list or something to turn checking off for certain nodes looks doable but I still think the style you are using is inconsistent indentation and trying to make the linter accept that (even through skipping object literals everywhere) is not a good practice.

Keep in mind that this would cover all ObjectExpression nodes which may be nested so don't need the exception at all. Looks like this is only a problem when you have all of the following conditions:

  • comma-first style
  • a top level var statement
  • indent level: 2 spaces
@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Aug 24, 2015

I like the exceptions idea. That will buy us some runway for styles we can't figure out how to support.

@nzakas nzakas added enhancement rule accepted and removed triage labels Aug 24, 2015

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Aug 24, 2015

I think we should think about making reusable "exception by node type" functionality. Indent already has a use-case of this with additional indentation.

@MarkKahn

This comment has been minimized.

Copy link
Author

MarkKahn commented Aug 25, 2015

I still think the style you are using is inconsistent indentation

@BYK - Of course it is, but it's a matter of priorities. It is impossible to have both consistent indentation and aligned keys with a comma-first style -- Either the first key will be too indented, the subsequent commas will be un-indented, or the keys won't line up. As the entire point here is about style, to me aligned keys come first in priority. Note that this isn't solely an issue with 2 spaces, as I don't believe half-unindenting a line with a 4-space rule is correct either (it just doesn't currently trigger a linter error)

+1 to general exceptions, or possibly a way to re-configure rules by node type, though I've no idea how much that would complicate either the code base or config files.

@keis

This comment has been minimized.

Copy link

keis commented Aug 26, 2015

The use of semicolons also seems to play a part

this fails on line 4

var foo = 1
    ,bar = 2

var baz = 3

while this passes

var foo = 1
    ,bar = 2;

var baz = 3

using this configuration

{
  "rules": {
    "comma-style": [2, "first"],
    "indent": 2
  }
}
@BYK

This comment has been minimized.

Copy link
Member

BYK commented Aug 26, 2015

@keis - This is a different issue addressed in #3515

@gyandeeps gyandeeps closed this in f8a7d31 Dec 19, 2015

nzakas added a commit that referenced this issue Dec 19, 2015

Merge pull request #4746 from eslint/issue4739
Fix: Comma first issue in `indent` (fixes #4739, fixes #3456)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

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