Rule proposal: `padding-line-between-component-options` #1817

barthy-koeln · 2022-03-16T18:57:37Z

Please describe what the rule should do:
Enforces empty lines between top-level options of Vue components.

What category should the rule belong to?

Enforces code style (layout)
Warns about a potential error (problem)
Suggests an alternate way of doing something (suggestion)
Other (please specify:)

Provide 2-3 code examples that this rule should warn about:

//✓ GOOD
export default {
  name: 'a-button',

  data() {
    return {
      // ...
    }
  },

  computed: {
   // …
  }
}

// ✗ BAD
export default {
  name: 'a-button',
  data() {
    return {
      // ...
    }
  },
  computed: {
   // …
  }
}

Additional context

Consistent format makes it easier to read, find, select, and extend.
The rule can enforce no empty lines between options (swap GOOD and BAD in the example).
I'm not sure about the term "options" but decided so because of the "Options API" name.
StackOverflow Question requesting this (5 Upvotes)
Github Comment requesting this (6 upvotes)

ota-meshi · 2022-03-16T23:03:04Z

Thank you for your proposal.
The new rule seems to have duplicate check contents with the new-line-between-multi-line-property rule.
https://eslint.vuejs.org/rules/new-line-between-multi-line-property.html

Do you have any ideas to clarify those roles?

barthy-koeln · 2022-03-17T00:20:42Z

@ota-meshi

This suggested rule focuses solely on 'top-level' options (i.e. data, props, computed, ...), while the new-line-between-multi-line-property rule applies to any nested object.

My team and I style components with padding lines in top-level, but not nested properties:

export default {
  name: 'QIcon',

  props: {
    value: {
      type: String,
      required: true
    },
    focused: {
      type: Boolean,
      default: false,
      required: true
    },
    label: String,
    icon: String
  },

  computed: {
    that() {
      return true
    },
    these () {
      return false
    }
  }
}

I think there is room for both behaviours, but this rule's behaviour could be toggled by an option (e.g. depth, topLevelOnly, ...) within new-line-between-multi-line-property.

Finally, the other rule's naming might be ambiguous, as I did not interpret "new-line" to be the same as an "empty line" or "padding line". I did search for existing similar rules but missed new-line-between-multi-line-property because of its name.

How would you suggest we proceed?

ota-meshi · 2022-03-17T21:30:11Z

@barthy-koeln

I think we need to deprecate the new-line-between-multi-line-property rule and add new rules to meet various style requirements.

The new rule should be able to set the always or never of padding for each scope (e.g. options, properties, all?).
For always, there is an additional option to apply only to multi-line properties.
I don't have an idea for a rule name.
What do you think?

cc @FloEdelmann

Finally, the other rule's naming might be ambiguous, as I did not interpret "new-line" to be the same as an "empty line" or "padding line". I did search for existing similar rules but missed new-line-between-multi-line-property because of its name.

Since there is no rule naming convention, the rule name is determined by the rule proposer, implementer, and reviewer when the rule is implemented. So the rule names may not be unified. However, it is difficult to make a breaking change to rename after adding a rule.

FloEdelmann · 2022-03-18T09:12:14Z

Ah, I reckoned there was a similar rule, but couldn't find it either.

I also support to deprecate new-line-between-multi-line-property and add a new rule with options for different scopes. My suggestion:

Rule name: vue/padding-lines-in-component-definition

Options:

Either object form:

"vue/padding-lines-in-component-definition": {
    // non-negative integer defining the number of empty lines between e.g. `name`, `data`, `computed`
    // "ignore" to not check the top level
    "topLevel": 0 | 1 | 2 | … | "ignore",

    // non-negative integer defining the number of empty lines between e.g. individual prop definitions, method definitions
    // "ignore" to not check the second level
    "secondLevel": 0 | 1 | 2 | … | "ignore",

    // non-negative integer defining the number of empty lines between e.g. computed getter/setter, watch options
    // "ignore" to not check the third level
    "thirdLevel": 0 | 1 | 2 | … | "ignore",

    // whether to treat multiple consecutive single-line properties (e.g. `name`, `inheritAttrs`) as a single block
    // and expect no padding lines between them
    "groupSingleLineProperties": true | false
}

Or integer form:

"vue/padding-lines-in-component-definition": 0 | 1 | 2 | … // shorthand that sets all `topLevel`, `secondLevel` and `thirdLevel`

Default:

{
    "topLevel": 1,
    "secondLevel": 1,
    "thirdLevel": 1,
    "groupSingleLineProperties": true
}

ota-meshi · 2022-03-18T09:42:22Z

We cannot add an option to specify the number of rows in the empty lines. Forcing more than one empty line conflicts with ESLint's core rule no-multiple-empty-lines.

https://eslint.org/docs/rules/no-multiple-empty-lines

The rule should only check if it has empty lines.
Therefore, I think it is better to make always, never, and ignore as options.

I don't think the name secondLevel is good.
I think watch and coupled may use a third level.

{
  computed: {
    myProp: {
      get () {
        // ...
      },

      set (newValue) {
        // ...
      }
    }
  },
  watch: {
    myProp: {
      handler (val, oldVal) {
        // ...
      },

      deep: true
    },
  }
}

ota-meshi · 2022-03-18T09:52:54Z

I noticed how I had to consider whether to check defineProps({}) etc.
Do you think the new rule will check for objects in defineProps({}) etc?
I think the name topLebelcan be confusing to the user if the rule checks it.
I think the name component-definition can be confusing to the user if the rule doesn't check it.

FloEdelmann · 2022-03-18T09:54:11Z

We cannot add an option to specify the number of rows in the empty lines.

That suggestion was based on the current options of new-line-between-multi-line-property, where we already allow specifying that.
What about projects where no-multiple-empty-lines is disabled and there should be more padding between top-level options than between second level options?

Therefore, I think it is better to make always, never, and ignore as options.

If we decide to not check for a specific number, these names are good. I also updated my comment to change null to "ignore".

I don't think the name secondLevel is good.

Unfortunately, I couldn't come up with a better name. What do you suggest instead?

I think watch and coupled may use a third level.

Ah, indeed. I added it to my comment.

ota-meshi · 2022-03-18T10:04:56Z

That suggestion was based on the current options of new-line-between-multi-line-property, where we already allow specifying that.

The number specified in the options of the new-line-between-multi-line-property rule is the minimum number of lines to be judged as multiple line property. It's not the number of empty lines.

Unfortunately, I couldn't come up with a better name. What do you suggest instead?

I haven't formed an opinion yet, but what about using the name options instead of the name topLevel?
I think the secondLevel and thirdLevel can be set to one, but I haven't come up with an idea for it good name yet.

ota-meshi · 2022-03-18T10:09:31Z

I think the secondLevel and thirdLevel can be set to one, but I haven't come up with an idea for it good name yet.

~~What about the name declarations?~~
I think that defines may be better. This option can also be applied to defineEmits() and defineProps().

FloEdelmann · 2022-03-18T10:17:00Z

The number specified in the options of the new-line-between-multi-line-property rule is the minimum number of lines to be judged as multiple property. It's not the number of empty lines.

Ah, you're right.

defineProps({})

~~Hmm, good point. Maybe splitting the rule up into separate rules would make sense? That'd also avoid having to find better names for topLevel / secondLevel / etc.~~

Nevermind, I changed my mind during writing this comment. This suggestion gets too messy.

Details

Each rule could then have one string option ("always" | "multiline-only" | "never").

padding-line-between-component-options: looks inside export default {…}, defineComponent({…})
padding-line-between-props: looks inside export default { props: {…} }, defineComponent({ props: {…} }), defineProps({…})
padding-line-between-emits: looks inside export default { emits: {…} }, defineComponent({ emits: {…} }), defineEmits({…})
padding-line-between-computed-properties: looks inside export default { computed: {…} }, defineComponent({ computed: {…} })
padding-line-between-methods: looks inside export default { methods: {…} }, defineComponent({ methods: {…} })
padding-line-between-computed-getter-setter: looks inside export default { computed: { foo: {…} } }, defineComponent({ computed: { foo: {…} } })
padding-line-between-watch-options: looks inside export default { watch: { foo: {…} } }, defineComponent({ watch: { foo: {…} } })

barthy-koeln · 2022-03-18T10:18:23Z

Since vue/padding-lines-in-component-definition would affect all objects within a component definition, I would suggest a "default" or "global" setting, overridden for specific keys:

"vue/padding-lines-in-component-definition": {
    "padding": "always" | "never" | "ignore"
    
    // defaults to value of "padding"
    "options": "always" | "never" | "ignore"

    // defaults to value of "padding"
    "computed": "always" | "never" | "ignore"

    // ...
}

FloEdelmann · 2022-03-18T10:49:37Z

@barthy-koeln's suggestion goes in a similar direction as the one that I rejected during writing. So maybe it's worth to go that route.

Combining my original suggestion with @barthy-koeln's:

1. Add a new rule with the following options:

"vue/padding-lines-in-component-definition": {
    // top level
    "options": "always" | "never" | "ignore", // e.g. between `props` and `data`, defaults to "always"

    // second level
    "emits": "always" | "never" | "ignore", // also checks `defineEmits`, defaults to "always"
    "props": "always" | "never" | "ignore", // also checks `defineProps`, defaults to "always"
    "computed": "always" | "never" | "ignore", // defaults to "always"
    "watch": "always" | "never" | "ignore", // defaults to "always"
    "methods": "always" | "never" | "ignore", // defaults to "always"

    // third level
    "computedOptions": "always" | "never" | "ignore", // e.g. between `get` and `set`, defaults to "always"
    "watchOptions": "always" | "never" | "ignore", // e.g. between `handler` and `deep`, defaults to "always"

    "groupSingleLineProperties": true | false, // defaults to true
} | "always" | "never" // shortcut to set them all

2. Deprecate the vue/new-line-between-multi-line-property rule (in favor of the new one).

Changes to @barthy-koeln's suggestion:

add groupSingleLineProperties option
no "padding" option; instead inherit from the default
add shortcut to set all at once

barthy-koeln · 2022-03-18T14:08:11Z

FYI I have some time off (18.03. - 28.03.) and would like to submit an implementation once we agree on a schema.

inker · 2022-03-19T03:16:58Z

See #1390

FloEdelmann · 2022-03-19T09:39:57Z

@ota-meshi @barthy-koeln What do you think about my last suggestion? I think if you both give it a thumbs-up, then it's ready to be implemented.

@inker Thanks for linking #1390. The consensus in both that issue and this one seems to be that it's necessary to add the possibility to specify the padding per scope. What do you think about the last suggestion?

ota-meshi · 2022-03-20T00:28:35Z

I think the options @FloEdelmann have suggested are mostly good.
However, I think it might be better to have nested options if you need to support Vue's custom options.
Nested options to prevent conflicting custom option names.

"vue/padding-lines-in-component-definition": {
    // top level
    "options": "always" | "never" | "ignore", // e.g. between `props` and `data`, defaults to "always"

    // second level
    "properties": { // There may be a better name.
        // Vue builtin
        "emits": "always" | "never" | "ignore", // also checks `defineEmits`, defaults to "always"
        "props": "always" | "never" | "ignore", // also checks `defineProps`, defaults to "always"
        "data": "always" | "never" | "ignore",
        "methods": "always" | "never" | "ignore",
        "computed": {
            // second level
            "keys": /*There may be a better name.*/ "always" | "never" | "ignore",
            // third level
            "options": /*There may be a better name.*/ "always" | "never" | "ignore",
        },
        // Custom
        "foo": "always" | "never" | "ignore",
        "bar": {
            "keys": "always" | "never" | "ignore",
            "options": "always" | "never" | "ignore",
        },
        // ..
   },

    "groupSingleLineProperties": true | false, // defaults to true
}

I haven't yet checked if there are any well-known custom options for which this rule should be applied.
At the very least, I think we should check Nuxt's custom options.

barthy-koeln · 2022-03-20T03:06:41Z

I like the freedom of configuration this provides, but I'm not happy with the names yet.

We always configure either padding lines between items or within each item. Here's an attempt to clarify this:

"vue/padding-lines-in-component-definition": {
    "betweenOptions": "always" | "never" | "ignore", // e.g. between `props` and `data`, defaults to 'always'

    "withinOption": {
        "emits": "always" | "never" | "ignore", // padding between two emit definitions

        "props": {
            "betweenItems": "always" | "never" | "ignore", // padding between two props
            "withinEach":  "always" | "never" | "ignore", // padding between `type`, `default`, `required`, ...
        } | "always" | "never" | "ignore", // shortcut to set both

        "computed": {
            "betweenItems": "always" | "never" | "ignore", // padding between two computed properties
            "withinEach":  "always" | "never" | "ignore", // padding between `get` and `set` methods
        } | "always" | "never" | "ignore", // shortcut to set both

        // ...
   },

    "groupSingleLineProperties": true | false, // defaults to true
} | "always" | "never", // shortcut to set all

This can be read as understandable sentences:

Padding lines between options.

Padding lines within the option emits

Padding lines between items of the computed option

Padding lines within each item of the computed option

barthy-koeln · 2022-04-06T16:55:33Z

Thanks @ota-meshi for the thumbs-up, can I understand this as an approval?

@FloEdelmann any further notes?

ota-meshi · 2022-04-06T23:31:46Z

Thanks @ota-meshi for the thumbs-up, can I understand this as an approval?

Yes! I think the option you proposed is good.

FloEdelmann · 2022-04-11T17:00:11Z

I don't have any further notes, the suggestions looks fine.

barthy-koeln mentioned this issue Mar 16, 2022

Add vue/padding-line-between-component-options rule #1818

Closed

FloEdelmann linked a pull request Mar 16, 2022 that will close this issue

Add vue/padding-line-between-component-options rule #1818

Closed

FloEdelmann added the new rule proposition label Mar 16, 2022

barthy-koeln changed the title ~~Rule proposal: empty-line-between-options~~ Rule proposal: padding-line-between-component-options Mar 17, 2022

ItMaga mentioned this issue Dec 1, 2022

Add padding-lines-in-component-definition rule #2052

Merged

FloEdelmann closed this as completed in #2052 Jan 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rule proposal: `padding-line-between-component-options` #1817

Rule proposal: `padding-line-between-component-options` #1817

barthy-koeln commented Mar 16, 2022 •

edited

Loading

ota-meshi commented Mar 16, 2022

barthy-koeln commented Mar 17, 2022

ota-meshi commented Mar 17, 2022

FloEdelmann commented Mar 18, 2022 •

edited

Loading

ota-meshi commented Mar 18, 2022

ota-meshi commented Mar 18, 2022

FloEdelmann commented Mar 18, 2022

ota-meshi commented Mar 18, 2022 •

edited

Loading

ota-meshi commented Mar 18, 2022 •

edited

Loading

FloEdelmann commented Mar 18, 2022

barthy-koeln commented Mar 18, 2022

FloEdelmann commented Mar 18, 2022 •

edited

Loading

barthy-koeln commented Mar 18, 2022

inker commented Mar 19, 2022

FloEdelmann commented Mar 19, 2022

ota-meshi commented Mar 20, 2022 •

edited

Loading

barthy-koeln commented Mar 20, 2022

barthy-koeln commented Apr 6, 2022

ota-meshi commented Apr 6, 2022

FloEdelmann commented Apr 11, 2022

Rule proposal: padding-line-between-component-options #1817

Rule proposal: padding-line-between-component-options #1817

Comments

barthy-koeln commented Mar 16, 2022 • edited Loading

ota-meshi commented Mar 16, 2022

barthy-koeln commented Mar 17, 2022

ota-meshi commented Mar 17, 2022

FloEdelmann commented Mar 18, 2022 • edited Loading

ota-meshi commented Mar 18, 2022

ota-meshi commented Mar 18, 2022

FloEdelmann commented Mar 18, 2022

ota-meshi commented Mar 18, 2022 • edited Loading

ota-meshi commented Mar 18, 2022 • edited Loading

FloEdelmann commented Mar 18, 2022

barthy-koeln commented Mar 18, 2022

FloEdelmann commented Mar 18, 2022 • edited Loading

barthy-koeln commented Mar 18, 2022

inker commented Mar 19, 2022

FloEdelmann commented Mar 19, 2022

ota-meshi commented Mar 20, 2022 • edited Loading

barthy-koeln commented Mar 20, 2022

barthy-koeln commented Apr 6, 2022

ota-meshi commented Apr 6, 2022

FloEdelmann commented Apr 11, 2022

Rule proposal: `padding-line-between-component-options` #1817

Rule proposal: `padding-line-between-component-options` #1817

barthy-koeln commented Mar 16, 2022 •

edited

Loading

FloEdelmann commented Mar 18, 2022 •

edited

Loading

ota-meshi commented Mar 18, 2022 •

edited

Loading

ota-meshi commented Mar 18, 2022 •

edited

Loading

FloEdelmann commented Mar 18, 2022 •

edited

Loading

ota-meshi commented Mar 20, 2022 •

edited

Loading