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

Integrate ESLint into our development and CI process #833

Merged
merged 11 commits into from
Feb 5, 2021
Merged

Integrate ESLint into our development and CI process #833

merged 11 commits into from
Feb 5, 2021

Conversation

philippwaller
Copy link
Contributor

@philippwaller philippwaller commented Jan 22, 2021

This PR integrates ESLint into our development and CI process. The following changes have been made:

  • Update ESLint and all dependencies
  • Integration of ESLint into the webpack build process
  • Adding ESLint rules to improve code quality and readability

I have made a preselection of rules that I believe would increase the overall code quality. Of course, this selection is very opinionated, so I'm looking forward to your feedback. The following rules are already activated and conform to the current code base.

Rule Level Description
constructor-super error Link
cypress/no-assigning-return-values error Link
cypress/no-async-tests error Link
cypress/no-unnecessary-waiting error Link
for-direction error Link
getter-return error Link
import/default error Link
import/export error Link
import/named error Link
import/namespace error Link
jsx-quotes error Link
linebreak-style error Link
no-async-promise-executor error Link
no-class-assign error Link
no-compare-neg-zero error Link
no-cond-assign error Link
no-const-assign error Link
no-constant-condition error Link
no-control-regex error Link
no-delete-var error Link
no-dupe-args error Link
no-dupe-class-members error Link
no-dupe-else-if error Link
no-dupe-keys error Link
no-duplicate-case error Link
no-empty-character-class error Link
no-empty-pattern error Link
no-ex-assign error Link
no-extra-boolean-cast error Link
no-extra-semi error Link
no-fallthrough error Link
no-func-assign error Link
no-global-assign error Link
no-import-assign error Link
no-inner-declarations error Link
no-invalid-regexp error Link
no-irregular-whitespace error Link
no-misleading-character-class error Link
no-mixed-spaces-and-tabs error Link
no-new-symbol error Link
no-obj-calls error Link
no-octal error Link
no-prototype-builtins error Link
no-redeclare error Link
no-regex-spaces error Link
no-self-assign error Link
no-setter-return error Link
no-shadow-restricted-names error Link
no-sparse-arrays error Link
no-this-before-super error Link
no-undef error Link
no-unexpected-multiline error Link
no-unreachable error Link
no-unsafe-finally error Link
no-unsafe-negation error Link
no-unsafe-optional-chaining error Link
no-unused-labels error Link
no-useless-escape error Link
no-whitespace-before-property error Link
no-with error Link
require-yield error Link
space-in-parens error Link
use-isnan error Link
valid-typeof error Link
vue/comment-directive error Link
vue/experimental-script-setup-vars error Link
vue/html-end-tags warn Link
vue/jsx-uses-vars error Link
vue/no-arrow-functions-in-watch error Link
vue/no-async-in-computed-properties error Link
vue/no-custom-modifiers-on-v-model error Link
vue/no-dupe-keys error Link
vue/no-dupe-v-else-if error Link
vue/no-duplicate-attributes error Link
vue/no-lone-template warn Link
vue/no-multiple-slot-args warn Link
vue/no-multiple-template-root error Link
vue/no-reserved-keys error Link
vue/no-shared-component-data error Link
vue/no-side-effects-in-computed-properties error Link
vue/no-spaces-around-equal-signs-in-attribute warn Link
vue/no-template-key error Link
vue/no-textarea-mustache error Link
vue/no-unused-components error Link
vue/no-unused-vars error Link
vue/no-use-v-if-with-v-for error Link
vue/no-v-for-template-key error Link
vue/no-v-model-argument error Link
vue/one-component-per-file warn Link
vue/prop-name-casing warn Link
vue/require-component-is error Link
vue/require-prop-type-constructor error Link
vue/require-render-return error Link
vue/require-v-for-key error Link
vue/require-valid-default-prop error Link
vue/return-in-computed-property error Link
vue/use-v-on-exact error Link
vue/v-bind-style warn Link
vue/valid-template-root error Link
vue/valid-v-bind error Link
vue/valid-v-bind-sync error Link
vue/valid-v-cloak error Link
vue/valid-v-else error Link
vue/valid-v-else-if error Link
vue/valid-v-for error Link
vue/valid-v-html error Link
vue/valid-v-if error Link
vue/valid-v-model error Link
vue/valid-v-on error Link
vue/valid-v-once error Link
vue/valid-v-pre error Link
vue/valid-v-show error Link
vue/valid-v-text error Link
vue/no-v-html off Link

The following rules would currently fail and are therefore temporarily disabled. However, I am of the opinion that these should also be activated in the medium term. Due to the large number of required changes, the activations should be clustered into multiple pull requests. I would follow up on that after we have a common agreement.

Rule Level Description
camelcase error Link
comma-dangle error Link
comma-spacing error Link
eol-last error Link
indent error, 2 Link
no-console error Link
no-debugger error Link
no-empty error Link
no-trailing-spaces error Link
no-unused-vars error Link
no-useless-catch error Link
quotes error, single Link
vue/attributes-order error Link
vue/component-tags-order error Link
vue/html-closing-bracket-newline error Link
vue/html-closing-bracket-spacing error Link
vue/html-indent error Link
vue/html-quotes error Link
vue/html-self-closing error Link
vue/max-attributes-per-line error Link
vue/multiline-html-element-content-newline error Link
vue/mustache-interpolation-spacing error Link
vue/no-multi-spaces error Link
vue/no-mutating-props error Link
vue/no-parsing-error error Link
vue/no-template-shadow error Link
vue/order-in-components error Link
vue/require-default-prop error Link
vue/require-prop-types error Link
vue/singleline-html-element-content-newline error Link
vue/this-in-template error Link
vue/v-on-style error Link
vue/v-slot-style error Link
vue/valid-v-slot error Link

Attention: After merging, these changes can impact the build success of pending pull requests.

@philippwaller philippwaller requested a review from a team as a code owner January 22, 2021 12:12
@philippwaller philippwaller marked this pull request as draft January 22, 2021 17:23
@philippwaller philippwaller marked this pull request as ready for review January 22, 2021 21:42
@ghys
Copy link
Member

ghys commented Jan 24, 2021

Thanks for taking care of this!
I was content with just having ESLint hints in the IDE with the extension, but since I had to remind PR authors a couple times to verify there were no linting errors, I suppose it's time to have it more strictly enforced.

About the second table of rules, some of them do indeed make sense but I'm hesitant about those:

  • no-console
  • those related to attributes: order, max per line etc.
  • the default component tags order (I prefer "template","style","script")
  • enforcing types and defaults in props
  • no-v-html cannot be enabled, we require it to display config parameter descriptions

@philippwaller
Copy link
Contributor Author

Here are my two cents:

no console
Loggin to the console might be great during development but should be avoided in production. It will reduce the overall application performance (especially in loops), increase the memory consumption and may reveal information that is not intended for the user.

Attribute related rules like order, max per line etc
I am a big fan of these rules. From my point of view, they ensure a consistent convention across all files and increase the overall readability. We have some Vue file whose lines have well over 200 characters. The record holder even has 376 characters per line (thing-details.vue:20). It's not fun to edit these files, even on my 32-inch 4k monitor.

Component tags order
Fair enough. I adjusted the rule accordingly.

Enforcing types and defaults in props
Strong typing and default values can prevent bugs, specially when multiple developers involved. Take the property thingId as an example. As a new developer, I need to review the code before I know whether a string or an integer is expected. I know it's general JavaScript problem, but it's a step in the right direction. (again, personal opinion)

no-v-html
Got you. Can't be enabled.

Since we are mainly talking about the rule that will not be activated for the time being, I suggest that we merge the commit and evaluate the changes in the respective pull requests. It is difficult to assess the impact without looking at concrete examples. What do think @ghys?

@ghys
Copy link
Member

ghys commented Feb 4, 2021

Yes of course I surely understand the rationale behind these rules, I was mainly in "RAD" mode for the entirety of this project so relatively lax on the code tidiness and it can be a little dirty at places, these rules would make it better. I'm using the Vue devtools extension, as you might imagine, to figure out what is going on, what are props set to, etc.
As you pointed out there are most involved rules that would require a lot of changes if enabled - and the inevitable regression testing, very important - if you're willing to come up with these, I will certainly not veto any.
Personal preference, I'd still like not to completely forbid console.log for the occasional important message in production - most of them were replaced with console.debug anyways - so we can probably check there's no unnecessary logging in code reviews instead of ESLint.
For the time being, we can start with the rules that are already enabled - you'll have to rebase though. Thanks for your effort!

Signed-off-by: Philipp Waller <1090452+philippwaller@users.noreply.github.com>
Signed-off-by: Philipp Waller <1090452+philippwaller@users.noreply.github.com>
Signed-off-by: Philipp Waller <1090452+philippwaller@users.noreply.github.com>
Signed-off-by: Philipp Waller <1090452+philippwaller@users.noreply.github.com>
Signed-off-by: Philipp Waller <1090452+philippwaller@users.noreply.github.com>
Signed-off-by: Philipp Waller <1090452+philippwaller@users.noreply.github.com>
Signed-off-by: Philipp Waller <1090452+philippwaller@users.noreply.github.com>
Signed-off-by: Philipp Waller <1090452+philippwaller@users.noreply.github.com>
…raries

Signed-off-by: Philipp Waller <1090452+philippwaller@users.noreply.github.com>
Signed-off-by: Philipp Waller <1090452+philippwaller@users.noreply.github.com>
@philippwaller
Copy link
Contributor Author

philippwaller commented Feb 5, 2021

we can probably check there's no unnecessary logging in code reviews instead of ESLint.

Sounds like a good alternative. I have adjusted the rules accordingly.

@ghys
Copy link
Member

ghys commented Feb 5, 2021

@philippwaller This seems to work well, one last little request: could you disable linebreak-style for the time being - this one caused me some problems. I'd prefer keeping the git behavior on Windows as it is described in https://eslint.org/docs/rules/linebreak-style#using-this-rule-with-version-control-systems which is fine, than adding a .gitattributes file for now.
We can consider re-adding it later.
Otherwise, LGTM!

Signed-off-by: Philipp Waller <1090452+philippwaller@users.noreply.github.com>
Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks!

@ghys ghys merged commit 6875575 into openhab:main Feb 5, 2021
@ghys ghys added enhancement New feature or request main ui Main UI labels Feb 5, 2021
@ghys ghys added this to the 3.1 milestone Feb 5, 2021
@philippwaller philippwaller deleted the code_quality branch February 5, 2021 15:01
// https://github.com/vuejs/eslint-plugin-vue#priority-a-essential-error-prevention
// consider switching to `plugin:vue/strongly-recommended` or `plugin:vue/recommended` for stricter rules.
'plugin:vue/essential',
'@vue/standard'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@philippwaller I just noticed you removed @vue/standard here!? Any reason why?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, it appears it's much more strict than before...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request main ui Main UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants