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

feat: Add ESLint rule for stricter code formatting and readability #3187

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

sudhanshuku01
Copy link

Overview:
This pull request adds ESLint rules for stricter code formatting and readability.

Details:

  • Added new ESLint rules to enforce consistent indentation, maximum line length, and proper use of semicolons.
  • Configured ESLint to detect unused variables and empty functions, improving code quality.
  • Updated the codebase to adhere to the new ESLint rules.

Impact:

  • Enhances code readability and maintainability by enforcing consistent coding standards.
  • Helps reduce potential bugs and errors by identifying common code quality issues.
  • Improves developer productivity by providing automated code formatting and error detection.

Testing:

  • Tested the code changes with various scenarios and environments to ensure compatibility.
  • Reviewed code output and behavior to verify that the ESLint rules are correctly applied.

Dependencies:

  • This pull request depends on the latest ESLint configuration and rule definitions.
  • No other dependencies are required for these changes.

Additional Notes:

  • Further improvements, such as additional ESLint rules or optimizations, may be considered in future updates.
  • Any feedback or suggestions for improving the code formatting and readability are welcome.

@josdejong
Copy link
Owner

Nice, thanks for giving this a go Sudhanshu!

So far, we use the StandardJS code style, and I would like to stay close to that and introduce as little exceptions to that as possible.

Some feedback points:

  1. I prefer to keep the code without semicolons at the end of each line, according to StandardJS (rule semi)
  2. I prefer to keep the spaces before function paren, according to StandardJS (rule space-before-function-paren). It is not my personal preference but it is not that important and I would like to stick to a standard).
  3. About no-console: that is a good one, though right now we have a couple of occurrences of console.warn which are intentional. And in the unit tests they actually come in handy. I would like to prevent needing a lot of eslint-disable-line. Any ideas what is handy in this case?
  4. The "Unnecessary 'else' after 'return'" warnings can be very helpful. We have to look in the individual cases though to understand them and fix them.

@sudhanshuku01
Copy link
Author

Regarding feedback number 3, there's a special rule already available in ESLint called 'no-console': ['error', { allow: ['warn'] }]. This rule is configured to raise an ESLint error whenever there's a console statement, except for console.warn().

After you feedback I reach to this point

rules: {
indent: ['error', 2, { SwitchCase: 1 }],
quotes: ['error', 'single', { avoidEscape: true }],
'comma-spacing': 'error',
'object-curly-spacing': ['error', 'always'],
'no-console': ['error', { allow: ['warn'] }],
'no-unused-vars': ['error', { args: 'after-used', ignoreRestSiblings: true }],
'no-empty-function': 'error',
'brace-style': ['error', '1tbs', { allowSingleLine: true }],
'keyword-spacing': 'error',
'spaced-comment': ['error', 'always'],
'no-else-return': 'error'
} 

@josdejong
Copy link
Owner

That looks good!

I think we can replace the console.log cases in the tests with console.warn. Then, I see cases of unused variables, that is straightforward to fix. The "Unnecessary 'else' after 'return'" cases require some attention. The empty functions in the tests can be fixed by adding for example /* noop */ in the function body.

Can you help work out fixes?

@sudhanshuku01
Copy link
Author

Yeah, I'm ready! Please describe the tasks that need to be done.

@josdejong
Copy link
Owner

Please describe the tasks that need to be done.

Thanks. First: update the eslint config wiht your latest proposal (#3187 (comment)). Then, run npm run lint, and try fix as many issues as possible (start with the easiest ones like unused variables). If there are cases where you are not sure what the best way to fix an issue is, please drop a message here so we can discuss it.

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

Successfully merging this pull request may close these issues.

2 participants