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

new cap: option capIsNewExceptionPattern is not behaving as documented #12874

Closed
l1bbcsg opened this issue Feb 5, 2020 · 8 comments
Closed

new cap: option capIsNewExceptionPattern is not behaving as documented #12874

l1bbcsg opened this issue Feb 5, 2020 · 8 comments

Comments

@l1bbcsg
Copy link

@l1bbcsg l1bbcsg commented Feb 5, 2020

Documentation for capIsNewExceptionPattern says the following:

allows any uppercase-started function names that match the specified regex pattern to be called without the new operator.

However, the actual behaviour seems to be that entire call expression is matched against the pattern, not just the function name.

Either capIsNewExceptionPattern should match only function name, or the documentation should reflect the actual behaviour.

I believe matching code with regex is fundamentally wrong and the first option is more sane solution. So, the documentation should stay as is, behaviour corrected and example updated, but that might be a breaking change.

Additionally, the example given as "correct code for this rule" is actually producing warnings (see demo), the reason being the regex configured does not match anything in the code at all.

Also other options of this rule may be poorly documented as well, for example capIsNewExceptions also states that it matches names, while actually it can match either name or full expression.

Tell us about your environment

Node version: v12.10.0
npm version: v6.10.3
Local ESLint version: v6.8.0 (Currently used)
Global ESLint version: Not found

Also same result on whatever version is currently deployed as demo.

Please show your full configuration:

Demo links

Expected no warnings: https://eslint.org/demo#eyJ0ZXh0IjoiLyplc2xpbnQgbmV3LWNhcDogW1wiZXJyb3JcIiwgeyBcImNhcElzTmV3RXhjZXB0aW9uUGF0dGVyblwiOiBcIl5CYXIkXCIgfV0qL1xuXG5mb28uQmFyKCk7Iiwib3B0aW9ucyI6eyJwYXJzZXJPcHRpb25zIjp7ImVjbWFWZXJzaW9uIjoxMCwic291cmNlVHlwZSI6Im1vZHVsZSIsImVjbWFGZWF0dXJlcyI6e319LCJydWxlcyI6e30sImVudiI6e319fQ==

Not expected to ignore:
https://eslint.org/demo#eyJ0ZXh0IjoiLyplc2xpbnQgbmV3LWNhcDogW1wiZXJyb3JcIiwgeyBcImNhcElzTmV3RXhjZXB0aW9uUGF0dGVyblwiOiBcIl5mb29cXC5CYXIkXCIgfV0qL1xuXG5mb28uQmFyKCk7Iiwib3B0aW9ucyI6eyJwYXJzZXJPcHRpb25zIjp7ImVjbWFWZXJzaW9uIjoxMCwic291cmNlVHlwZSI6Im1vZHVsZSIsImVjbWFGZWF0dXJlcyI6e319LCJydWxlcyI6e30sImVudiI6e319fQ==

Documentation example, wrong on several accounts: https://eslint.org/demo#eyJ0ZXh0IjoiLyplc2xpbnQgbmV3LWNhcDogW1wiZXJyb3JcIiwgeyBcImNhcElzTmV3RXhjZXB0aW9uUGF0dGVyblwiOiBcIl5QZXJzb25cXC4uXCIgfV0qL1xuXG52YXIgZnJpZW5kID0gcGVyc29uLkFjcXVhaW50YW5jZSgpO1xudmFyIGJlc3RGcmllbmQgPSBwZXJzb24uRnJpZW5kKCk7Iiwib3B0aW9ucyI6eyJwYXJzZXJPcHRpb25zIjp7ImVjbWFWZXJzaW9uIjoxMCwic291cmNlVHlwZSI6Im1vZHVsZSIsImVjbWFGZWF0dXJlcyI6e319LCJydWxlcyI6e30sImVudiI6e319fQ==

What did you do?

Configuration:

eslint new-cap: ["error", { "capIsNewExceptionPattern": "^Bar$" }]

Code:

foo.Bar();

What did you expect to happen?

No warnings as function named Bar should be ignored.

What actually happened? Please include the actual, raw output from ESLint.

A warning against function Bar.

Are you willing to submit a pull request to fix this bug?

Only if it's as trivial as pattern.test(calleeName), but then it's probably trivial for anyone else as well.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Feb 5, 2020

Hi @l1bbcsg, thanks for the issue!

This is working as expected (ref #2584), but the documentation should be definitely improved to clarify the options and match the actual behavior (and to fix the mistake you found, "^Person\.." should be lowercase).

PR to fix and improve the documentation is welcome!

The options are designed to allow users to define exceptions by object names, property names or a combination of both. A configuration for the example you provided could be:

/*eslint new-cap: ["error", { "capIsNewExceptionPattern": "\.Bar$" }]*/

foo.Bar(); // no error

Online Demo

@naima-shk
Copy link

@naima-shk naima-shk commented Feb 24, 2020

@mdjermanovic Can i get this issue please ? :)

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Feb 24, 2020

Of course, PR is welcome :)

@jordanmoore753
Copy link

@jordanmoore753 jordanmoore753 commented Mar 12, 2020

I'm not sure if this issue has already been addressed but just not closed yet, but if not, may I tackle the issue of improving the documentation?

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Mar 12, 2020

I'm not sure if this issue has already been addressed but just not closed yet, but if not, may I tackle the issue of improving the documentation?

@jordanmoore753 there is no PR yet, and it doesn't look like anyone is working on this at the moment, so feel free to claim this :)

@eslint-deprecated
Copy link
Contributor

@eslint-deprecated eslint-deprecated bot commented May 7, 2020

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that accepted issues failing to be implemented after 90 days tend to
never be implemented, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented May 8, 2020

Reopening since we have a PR for this issue.

@eslint-deprecated
Copy link
Contributor

@eslint-deprecated eslint-deprecated bot commented Jun 9, 2020

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that accepted issues failing to be implemented after 90 days tend to
never be implemented, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

mdjermanovic added a commit that referenced this issue Jun 26, 2021
* Docs: fix and add more examples for `new-cap` rule

* Docs: update

* Docs: update

* Docs: update

* Docs: Update docs/rules/new-cap.md

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants