tlvince / eslint-plugin-jasmine

ESLint rules for Jasmine
https://www.npmjs.com/package/eslint-plugin-jasmine
MIT License
95 stars 57 forks source link

Potentially confusing rule configuration #374

Open studioromeo opened 2 months ago

studioromeo commented 2 months ago

Is your feature request related to a problem? Please describe. In the documentation for this plugin the list of recommended rules I find confusing https://github.com/tlvince/eslint-plugin-jasmine?tab=readme-ov-file#rules

The main issue is some rules are turned off (0) but its difficult to understand why thats the case. For example for no-describe-variables this rules description is that declaring variables in describe blocks can cause memory leaks which seems like a rule that should be enabled as an error since we don't want our test suite to be littered with memory leaking code.

But it's turned off which made me wonder why, I couldn't seem to find anything in the rule or the main readme about why this was the case. Looking into the blame for the readme I could see this commit https://github.com/tlvince/eslint-plugin-jasmine/commit/1182f0d50eae89d9c4fe2ce2dd23cc377e98720b but this only adds it into the readme and doesn't explain why its disabled by default.

After looking elsewhere and searching into jasmines history I could see that this issue has been fixed since version 3.0 https://github.com/jasmine/jasmine/issues/1154 so perhaps thats why this rule has been disabled?

Describe the solution you'd like

It would be good for disabled rules to have an explanation of why they are disabled unless its obviously a personal preference rule. If theres suggestions of memory leaks etc then this should be explained in the rule description as well as what versions of jasmine this is known to affect.

Additional context

Sorry for nitpicking this is a great project overall but as a newbie to the best practices for jasmine I found myself very confused on why certain rules weren't being applied.

Also a minor concern is the use of warnings over error state, I know this can be a contentious subject but according to the eslint documentation it seems like they would prefer rules to be errors by default and only use warnings as a migration path for new rules that eventually are set to error or for rules that perhaps aren't clear cut and need a human eye to confirm or deny the issue.

Rules are typically set to "error" to enforce compliance with the rule during continuous integration testing, pre-commit checks, >and pull request merging because doing so causes ESLint to exit with a non-zero exit code.

If you don’t want to enforce compliance with a rule but would still like ESLint to report the rule’s violations, set the severity to >"warn". This is typically used when introducing a new rule that will eventually be set to "error", when a rule is flagging something >other than a potential buildtime or runtime error (such as an unused variable), or when a rule cannot determine with certainty that >a problem has been found (when a rule might have false positives and need manual review).

https://eslint.org/docs/latest/use/configure/rules#rule-severities

Happy to raise pull requests to help resolve this but I wanted to first raise this issue to see what the maintainers thoughts and opinions are on what I've raised.

Thank you for your time.