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

key-spacing for one-liner object literals #4697

Closed
neverfox opened this Issue Dec 14, 2015 · 18 comments

Comments

Projects
None yet
5 participants
@neverfox
Copy link

neverfox commented Dec 14, 2015

Would it be possible to have a way to enforce different key-spacing rules depending on whether or not the object is specified on one line or multiple lines? A style we use is something like this:

// [2, { "align": "colon", "beforeColon": true, "afterColon": true }]
const obj1 = {
  key1     : 'val1',
  longKey2 : 'val2'
}

But for one-liners, we don't want beforeColon: true:

const obj2 = { key1: 'val1', key2: 'val2' }

The reason for this style choice is that we prefer no space before colons unless the colons can align, which is only relevant in multi-line situations. Right now, there doesn't seem to be a way to achieve this with a set of rules.

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Dec 14, 2015

@neverfox Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Dec 14, 2015

@neverfox

This comment has been minimized.

Copy link
Author

neverfox commented Dec 14, 2015

  1. This rule will warn when spacing in single-line objects does not match the specified options for single-line objects. I believe this would have to somehow extend the key-spacing rule in order not to run afoul of the requirements for new rules.
  2. stylistic
  3. I will need to learn more about custom rules to see if that can handle this case
  4. Sure
@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 14, 2015

This is one of our more complicated rules, so I'm not entirely sure if we can support this option. @btmills what do you think?

@btmills

This comment has been minimized.

Copy link
Member

btmills commented Dec 14, 2015

This wouldn't be too terribly difficult to implement as custom rules. Basically copy & paste the existing rule twice into two custom rules: single-line-key-spacing and multi-line-key-spacing. Add a quick check on the object literal in each whether it spans multiple lines. Then they can be configured separately.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 14, 2015

@btmills are you saying this wouldn't be a good candidate to include in the current key-spacing rule?

@btmills

This comment has been minimized.

Copy link
Member

btmills commented Dec 17, 2015

@neverfox Would custom rules be sufficient for your use case? I'd be happy to provide guidance for implementing them over in our Gitter chat.

@nzakas Implementing as two custom rules is trivial, but integrating this functionality into the one rule would not be. I'm hoping custom rules will work because I don't think the additional complexity meets the bar for inclusion.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 17, 2015

Got it. Should we instead create two core rules?

@btmills

This comment has been minimized.

Copy link
Member

btmills commented Dec 17, 2015

property-spacing and object-alignment?

Edit: property-spacing handles beforeColon and afterColon on single-line literals and optionally multi-line literals if you want an exact spacing configuration. object-alignment handles vertical alignment of properties in multi-line literals. If you want alignment, only use property-spacing for single-line literals.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 19, 2015

Sorry, I'm a bit lost. Are you proposing two new rules here?

@btmills

This comment has been minimized.

Copy link
Member

btmills commented Dec 19, 2015

Is that not what you meant when you said "Should we instead create two core rules?" I may have misunderstood.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 21, 2015

Yes, but I didn't know if you were saying these already exist or you were proposing new ones. That's why I asked if you were proposing new ones. :)

@btmills

This comment has been minimized.

Copy link
Member

btmills commented Dec 21, 2015

Ah, yes :) those would deprecate and eventually replace key-spacing (it's probably too late to both deprecate and remove it before 2.0). What do you think about those? There would be some duplication of both configuration and implementation unfortunately.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 21, 2015

Yeah, I'd say it's too late for 2.0.0 at this point. Is there any reason the current rule can't be extended to have separate configurations for each circumstance?

@btmills

This comment has been minimized.

Copy link
Member

btmills commented Dec 21, 2015

If we wanted to keep it as part of the same rule, I imagine configuration would look something like this:

{
    "key-spacing": [2, {
        "singleLine": {
            "beforeColon": false,
            "afterColon": true
        },
        "multiLine": {
            "beforeColon": true,
            "afterColon": true,
            "align": "colon"
        }
    }]
}

That would be in addition to the current schema. multiLine would basically point back to the current schema, and singleLine would have everything except align. Underneath, given the current options, we'd probably just copy them to identical singleLine and multiLine options that the rule would reference when verifying spacing and alignment. So it can certainly be done.

Edited to change single-line and multi-line to singleLine and multiLine as suggested below.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 22, 2015

I'd use singleLine and multiLine in the config, but otherwise looks good to me. Are you interested in making that change?

@btmills

This comment has been minimized.

Copy link
Member

btmills commented Dec 28, 2015

I can, but I have no idea when I'll have time. Anyone should feel free to pick this up.

@rpatil26

This comment has been minimized.

Copy link
Contributor

rpatil26 commented Jan 8, 2016

I am going to give it a try today.

rpatil26 added a commit to rpatil26/eslint that referenced this issue Jan 9, 2016

Update: Support for separate multi line and single line options (fixes
…eslint#4697)

Besides default (existing) options, we can specify ‘singleLine’ and
‘multiLine’ options.

rpatil26 added a commit to rpatil26/eslint that referenced this issue Jan 9, 2016

Update: Support for separate multi line and single line options (fixes
…eslint#4697)

Besides default (existing) options, we can specify ‘singleLine’ and
‘multiLine’ options.

rpatil26 added a commit to rpatil26/eslint that referenced this issue Jan 9, 2016

Update: Support for separate multi line and single line options (fixes
…eslint#4697)

Besides default (existing) options, we can specify ‘singleLine’ and
‘multiLine’ options.

rpatil26 added a commit to rpatil26/eslint that referenced this issue Jan 9, 2016

Update: Support multiLine and singleLine options (fixes eslint#4697)
Besides default (existing) options, we can specify ‘singleLine’ and
‘multiLine’ options.
@rpatil26

This comment has been minimized.

Copy link
Contributor

rpatil26 commented Jan 9, 2016

This was a bit tricky since we have to keep existing options for backward compatibility and also support new categorized options singleLine and multiLine with fallback.

I hope I got it right. I also wanted to validate schema such that if multiLine option is specified it should not allow existing options at sibling level. I don't know how can I write schema to do so.

rpatil26 added a commit to rpatil26/eslint that referenced this issue Jan 10, 2016

Update: Support multiLine and singleLine options (fixes eslint#4697)
Besides default (existing) options, we can specify ‘singleLine’ and
‘multiLine’ options.

rpatil26 added a commit to rpatil26/eslint that referenced this issue Jan 10, 2016

Update: Support multiLine and singleLine options (fixes eslint#4697)
Besides default (existing) options, we can specify ‘singleLine’ and
‘multiLine’ options.

rpatil26 added a commit to rpatil26/eslint that referenced this issue Jan 12, 2016

Update: Support multiLine and singleLine options (fixes eslint#4697)
Besides default (existing) options, we can specify ‘singleLine’ and
‘multiLine’ options.

rpatil26 added a commit to rpatil26/eslint that referenced this issue Jan 12, 2016

Update: Support multiLine and singleLine options (fixes eslint#4697)
Besides default (existing) options, we can specify ‘singleLine’ and
‘multiLine’ options.

rpatil26 added a commit to rpatil26/eslint that referenced this issue Jan 13, 2016

Update: Support multiLine and singleLine options (fixes eslint#4697)
Besides default (existing) options, we can specify ‘singleLine’ and
‘multiLine’ options.

@btmills btmills closed this in b1360da Jan 13, 2016

btmills added a commit that referenced this issue Jan 13, 2016

Merge pull request #4895 from rpatil26/issue4697
Update: Support multiLine and singleLine options (fixes #4697)

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

@eslint eslint bot added the archived due to age label Feb 6, 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.