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

Improve and introduce measures for consistent code style #4094

Open
wants to merge 12 commits into
base: 0_15
Choose a base branch
from

Conversation

Mattstir
Copy link
Contributor

@Mattstir Mattstir commented Aug 9, 2024

I'm highly passionate about clean code and a consistent code style, so I see this as a great opportunity to enhance WLED. 😉

By introducing ESLint, we can automatically enforce code style during each CI run (and hint at problems during a local npm build), ensuring that future changes adhere to the established guidelines. For now, I've aligned the rules with the suggestions in WLED's CONTRIBUTING.md and made them mandatory. This is now only a basic start, these rules can be easily expanded in the future to include more than just style.

I've also corrected the existing style issues in the .js files and js snippets in html files.

Note: This PR is purely stylistic with no logic changes. I hope you like it! 😄

@Mattstir
Copy link
Contributor Author

Mattstir commented Aug 9, 2024

Also note that each style rule got introduced in its own commit so we could easily revert one of them if not wanted ;)

@softhack007
Copy link
Collaborator

softhack007 commented Aug 9, 2024

@Mattstir interesting idea 😃

Two questions come to my mind:

  • are these rules only applied to JS and html? Because we already have a problem with flash size, and additional "beautification" in the embedded webpages could lead to increased flash size (bad).
  • do you have a list of the rules? Because I think it's better to first agree on the rule set, and then implement automated checks as a second step.
@Mattstir
Copy link
Contributor Author

Mattstir commented Aug 9, 2024

@softhack007 Thanks for your interest :D

  • With this pr applied to .js files and the js snippets that are inside the html files. Of course, bundle size is a very important issue and I was thinking about that when I added this. After each rule I added, I ran npm build, but nothing changed in the built files. So I assume that the minifying tool you guys use html-minifier-terser is already stripping whitespaces and comments (I haven't fully researched how you configure it now, but as I basically only changed whitespaces and these seem to be stripped again, I'm optimistic 🤩).

  • You can find the rules I added in the configuration file I added eslint.config.mjs.
    First, I extended the recommended rules from https://eslint.org/docs/latest/rules/ (see the rules that have ✅), but I switched some of them to off or just to warn not to flag any current code with errors.
    Then I added some style rules from https://eslint.style/rules

    "@stylistic/js/function-call-spacing": ["error", "never"],
    "@stylistic/js/key-spacing": "error",
    "@stylistic/js/keyword-spacing": "error",
    "@stylistic/js/no-extra-semi": "error",
    "@stylistic/js/no-whitespace-before-property": "error",
    "@stylistic/js/space-before-blocks": "error",
    "@stylistic/js/space-infix-ops": "error",
    "@stylistic/js/spaced-comment": "error"

I selected these based on the desired style in the CONTRIBUTING.md file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants