Formatting of the WebUI code base

[eikowagenknecht]

The problem

1. Formatting
   Currently there are lots of different formatting styles in the WebUI code base. Example from app.vue:

Sometimes very long lines

<f7-app v-if="init" :style="{ visibility: (($store.getters.user || $store.getters.page('overview')) || loginScreenOpened) ? '' : 'hidden' }" :params="f7params" :class="{ 'theme-dark': this.themeOptions.dark === 'dark', 'theme-filled': this.themeOptions.bars === 'filled' }">

Sometimes split by attribute.. mostly:

          <f7-list-item v-for="page in pages" :animate="false" :key="page.uid"
                        :class="{ currentsection: currentUrl.indexOf('/page/' + page.uid) >= 0 }"
                        :link="'/page/' + page.uid"
                        :title="page.config.label" view=".view-main" panel-close>

Formatting inconsistencies are present in HTML, JS, Stylus. This also makes the use of tools like VS Code "Format Document" hard to impossible.

Especially for beginners, it makes the code sometimes unneccessarily hard to read and comprehend.

2. ESLint rules
   In addition quite a few code quality ESLint rules are disabled. That leads for example to having a faulty html checked in right now (in app.vue there is a duplicate ).

Your suggestion

I could make a wordy case for why style guides are good here, but I think the guys from Prettier have summed it up quite well, so I'll refer to them instead: https://prettier.io/docs/en/why-prettier.html.

Prettier is an opinionated formatter that leaves only very few settings to be made, so the resulting code style is quite wide spread and easily recognizable for developers coming from other projects. For more information, see the link above.

Most Vue development is done with VSCode, Vetur package installed and the default formatting there is also to use Prettier, so it's integrated nicely with the dev environment used here.

I've toyed around with the ESLint settings a bit and a combination of using Prettier for HTML and JS and Stylus supremacy for stylus seems to work quite well.

ESLint

  extends: [
    'eslint:recommended',
    'plugin:cypress/recommended',
    'plugin:vue/recommended',
    'prettier' // package eslint-config-prettier
  ],

  rules: {
    'no-empty': ['error', { 'allowEmptyCatch': true }],
    'no-unused-vars': 'off',
  }

VSCode config:

  "vetur.validation.template": false,
  "vetur.format.defaultFormatter.js": "prettier",
  "vetur.format.defaultFormatter.html": "prettier",
  "stylusSupremacy.insertBraces": false,
  "stylusSupremacy.insertColons": false,
  "stylusSupremacy.insertSemicolons": false,

The code examples above would for example be formatted to:

<f7-app
    v-if="init"
    :style="{
      visibility:
        $store.getters.user ||
        $store.getters.page('overview') ||
        loginScreenOpened
          ? ''
          : 'hidden',
    }"
    :params="f7params"
    :class="{
      'theme-dark': this.themeOptions.dark === 'dark',
      'theme-filled': this.themeOptions.bars === 'filled',
    }"
  >

          <f7-list-item
            v-for="page in pages"
            :animate="false"
            :key="page.uid"
            :class="{
              currentsection: currentUrl.indexOf('/page/' + page.uid) >= 0,
            }"
            :link="'/page/' + page.uid"
            :title="page.config.label"
            view=".view-main"
            panel-close
          >

This has the added benefit that making changes is easier to track in git, because attributes are on their own line.

Regarding ESLint:
The ESLint rules with most errors are:

• vue/attributes-order
• vue/component-tags-order
• vue/order-in-components
• vue/attribute-hyphenation
• vue/no-mutating-props
• vue/this-in-template
• vue/require-prop-types
• no-case-declarations

I think fixing these would be quite beneficial to the code base. Maybe there is some odd case where it's needed to break those rules, but that can be inline-commented for each occasion.

Of course there would need to be one major reformatting and enforcing of the rules and yes, open PRs would need to be refactored accordingly. But there are not so many open at the moment, so I think it should be as good a moment to do this as it will ever be ;-)

I see you have been working on ESLint recently, @ghys (e.g. #942). What is your stance on this?

I'd be happy to do the formatting and rule fixing for the whole code base, if you agree that it should be done.

[ghys]

This was already started in #833 by @philippwaller and I believe the reason some ESLint rules were disabled was apparently because there were too many errors to fix at once (second table in #833).

openhab-webui/bundles/org.openhab.ui/web/.eslintrc.js

Lines 85 to 86 in 4c0f9fe

	// The following rules should be activated successively. Due to the large amount
	// of required changes, the activations should be clustered in several pull requests.

• Regarding the enforcement of one attribute per line on start tags, and the assertion that it leads to more readable code, this was discussed in Activation of template related ESLint rules #905 (comment) and I remain largely unconvinced. It makes sense in a lot of cases, true, but in others it just expands the template vertically and IMHO you have a harder time matching the end tags to the start ones. Enforcing these rules would lead to examples like in the above comment. I'm more of the opinion to fix the instances where it's really bad (lines that are really too long) and prefer more compact "blocks" or sections.
• vue/attributes-order - not feeling as strongly as the former one except that that as you organically add your attributes you'll get order attribute errors constantly and will have to re-arrange them (either automatically or manually), this disrupts the workflow and doesn't add that much value IMHO
• vue/order-in-components - yes with template, style and script (different than the default)
• vue/attribute-hyphenation - yes
• vue/no-mutating-props - yes although there are a lot of instances of this, and it's not necessarily always an anti-pattern (https://vuejs.org/v2/style-guide/#Implicit-parent-child-communication-use-with-caution)
• vue/this-in-template - yes
• vue/require-prop-types - OK I suppose but needs a lot of work
• no-case-declarations - didn't know there were cases of these but yes

Also I absolutely want to keep the @vue/standard rules at least for now, which I believe are incompatible with Prettier. If there were a way to have both then why not.
Adding Stylus however rules I think would be a big plus!
Note that running ESLint with the dev server was disabled because it would lead to up to 30 seconds delays when saving a file. However it will be run as a GitHub action and errors are displayed in PRs (also the production builds will fail).

[eikowagenknecht]

Thanks for taking the time to reply in detail what you think about this issue.

I think we have to split this topic into 2 areas:

1. Formatting (everything "layout", changing this does not alter the meaning of the code)
2. Linting (code quality)

Historically, ESLint also has lots of rules that fall into the "formatting" category and (as you noted) sometimes conflict with prettier. Fortunately there is a plugin (https://github.com/prettier/eslint-config-prettier) that disables all formatting related rules from vue and leaves the formatting to prettier. Prettier can also be integrated in the eslint workflow as a eslint pluging (https://github.com/prettier/eslint-plugin-prettier#recommended-configuration). You can see the disabled rules here: https://github.com/prettier/eslint-config-prettier/blob/main/index.js

Applicated to the current config the following rules would be removed because they are only formatting related and might conflict with prettier:

    // 'arrow-parens': 'off',
    // 'comma-dangle': 'error',
    // 'comma-spacing': 'error',
    // 'eol-last': 'error',
    // 'generator-star-spacing': 'off',
    // 'indent': ['error', 2, { 'SwitchCase': 1 }],
    // 'jsx-quotes': 'error',
    // 'linebreak-style': 'off',
    // 'multiline-ternary': 'off',
    // 'no-trailing-spaces': 'error',
    // 'no-whitespace-before-property': 'error',
    // 'quote-props': 'off',
    // 'space-in-parens': 'error',
    // 'vue/html-closing-bracket-newline': ['error', { 'singleline': 'never', 'multiline': 'never' }],
    // 'vue/html-closing-bracket-spacing': 'error',
    // 'vue/html-indent': 'error',
    // 'vue/html-quotes': 'error',
    // 'vue/html-self-closing': 'error',
    // 'vue/max-attributes-per-line': 'off',
    // 'vue/multiline-html-element-content-newline': 'error',
    // 'vue/mustache-interpolation-spacing': 'error',
    // 'vue/no-multi-spaces': 'error',
    // 'vue/singleline-html-element-content-newline': 'error',
    // 'quotes': ['error', 'single'],

Regarding 1)

• I can see why you say that sometimes the one-attribute-per-line layout of prettier is a bit too verbose / leads to too many lines. But even in the example posted here I prefer the prettier formatted version because it allows me to grasp the attributes faster. But that's just personal preference.
• From a cost-to-benefit ratio perspective the "only fix really bad lines" approach is quite problematic becaue
  • a) we need to define what is "too bad" (again quite opinionated)
  • b) someone needs to actually do this manually or only half automated and mostly
  • c) new developers will not automatically have the same definition of "bad line" which will lead to more request in PRs to fix the formatting if you deem it too long or letting it go because it's too much trouble to get someone to correct this just for the style
• So I still very much prefer the "have a code that's formatted about 95% as good as we want and never care or discuss about formatting again" option, freeing up capacity to improve the contents of the code

Regarding 2)

• Why do you want to keep @vue/standard (https://github.com/vuejs/eslint-config-standard#readme)? It seems rather unusual to me to use this in addition to the "base" / "essentials" / "strongly-recommended" / "recommended" rules from the official vue plugin (https://eslint.vuejs.org/user-guide/#installation).
• With @vue/standard (and no custom rules (!)) there are 455 errors and 2331 warnings, without 75 errors and 2331 warnings.
• So @vue/standard activates some non-vue-namespace rules like "camelcase", "no-console", "dot-notation" and "prefer-const". Those are then disabled with custom rules anyways, e.g. "prefer-const: off". I think we should add the ones we want sparingly with good reason as custom rules here.
• With the disabled rules and the abovementioned eslint config that disables conflicting prettier rules, there doesn't seem to be a problem with @vue/standard either if we really need to keep it for some reason.

The following rules are already included in vue recommended, no need to declare seperately:

    // 'vue/v-on-style': 'error',
    // 'vue/v-slot-style': 'error',

The following could be also activated (= removed here) if agreed upon:

    'no-debugger': 'off', // https://eslint.org/docs/rules/no-debugger, eslint recommended, no occurrences in current code base
    'one-var': 'off', // https://eslint.org/docs/rules/one-var, could easily be turned on, few occasions to be fixed
    'prefer-promise-reject-errors': 'off', // https://eslint.org/docs/rules/prefer-promise-reject-errors, improve traceability
    'vue/attribute-hyphenation': 'off', // https://eslint.vuejs.org/rules/attribute-hyphenation.html, improve readability, included in vue strongly recommended
    'vue/attributes-order': 'off', // https://eslint.vuejs.org/rules/attributes-order.html, improve readability, included in vue recommended
    'vue/component-definition-name-casing': 'off', // https://eslint.vuejs.org/rules/component-definition-name-casing.html, included in vue strongly recommended

Remaining project specific rules:

  'dot-notation': 'off', // https://eslint.org/docs/rules/dot-notation, could be turned on with allowPattern set to alow underscore use
  'import/default': 'error',
  'import/export': 'error',
  'import/extensions': 'off',
  'import/first': 'off',
  'import/named': 'error',
  'import/namespace': 'error',
  'import/no-extraneous-dependencies': 'off',
  'import/no-unresolved': 'off',
  'no-case-declarations': 'off', // https://eslint.org/docs/rules/no-case-declarations, from eslint recommended set
  'no-console': 'off', // https://eslint.org/docs/rules/no-console, discussed in https://github.com/openhab/openhab-webui/pull/833
  'no-unsafe-optional-chaining': 'error', // https://eslint.org/docs/rules/no-unsafe-optional-chaining
  'vue/no-v-html': 'off', // Can't be enabled, see https://github.com/openhab/openhab-webui/pull/833#issuecomment-766420549

So how should we progress further with this? I can add stylus rules in a first PR since that seems not to need further discussion :-)

I could also start with fixing some of the rules we already agree on (so that should be done anyways), but I think going through the full prettier formatting first would make the subsequent PRs already better readable (e.g. reordering lines should be easier to track) so I'm waiting on your feedback for now.

[kubawolanin]

Hey! I started playing around with prettier on this repo long time ago (Feb 2020) but haven't finished it (still dealing with cancer).
kubawolanin@137f00b

Improvements there include adding pretty-quick and fix-changed scripts, that run eslint along with pretty-quick for all changed files, which saves time by a lot.

    "test:e2e:gui": "npx cypress open",
    "lint": "eslint ./src",
    "fix-changed": "npm-run-all lint:fix-changed prettier:fix-changed",
    "prettier:fix-changed": "pretty-quick --write --changedSince=origin/master",
    "lint:fix-changed": "npm run lint --fix --changedSince=origin/master",
    "pretty-quick": "pretty-quick"

I'll see if there's something to salvage from this branch and make a proper PR, but first it would be great to consolidate what has been agreed on by you @eikowagenknecht and @ghys :-)

Cheers!

[eikowagenknecht]

Closing my issues with openHAB that seem to have gone stale.