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

fix: Object destructuring in function parameters if key is string #599

Merged

Conversation

michalczaplinski
Copy link
Contributor

@michalczaplinski michalczaplinski commented Jul 1, 2020

Fixes: #598

A key in a destructured object that is passed as a function parameter could be a string or a number. This case was previously not handled by getParamName().

Example that failed before and succeeds now:

const someFunction = ({ 
  "value": renamedValue,  // eslint throws Unsupported function signature format.
  otherArg,
}) => {
  console.log("not important");
}

README.md Outdated Show resolved Hide resolved
@michalczaplinski
Copy link
Contributor Author

Unrelated to the current PR: I've had to push with git push --no-verify because otherwise eslint would yell at me about errors in unrelated files like:


/Users/czapla/Testing/eslint-plugin-jsdoc/src/iterateJsdoc.js
  44:23  error  There should be a linebreak after this element  array-element-newline
  44:33  error  There should be a linebreak after this element  array-element-newline
  44:44  error  There should be a linebreak after this element  array-element-newline
  44:54  error  There should be a linebreak after this element  array-element-newline
  45:22  error  There should be a linebreak after this element  array-element-newline
  45:33  error  There should be a linebreak after this element  array-element-newline
  45:42  error  There should be a linebreak after this element  array-element-newline
  45:53  error  There should be a linebreak after this element  array-element-newline

Are you aware of that?

@brettz9
Copy link
Collaborator

brettz9 commented Jul 5, 2020

To just respond on the linting issue for now...

Ah yes, wasn't seeing this until npm update --depth 20, updating my package-lock.json copy.

@gajus:

  1. Can we add package-lock.json to the repo (ignoring for npm releases) so devs can be on the same page? It only causes lock-in for local npm install use.
  2. IMV, the array-element-newline rule can be pretty oppressive, e.g., I hardly find any problem with the following, yet would find adding line breaks to each line to be very bloated (and stuffing it all on one line to be annoying for those of us avoiding wrapping as a default on our IDEs):
          if ([
            'example', 'return', 'returns', 'throws', 'exception',
            'access', 'version', 'since', 'license', 'author',
            'default', 'defaultvalue',
          ].includes(data.tag)) {

@brettz9 brettz9 force-pushed the fix-destructuring-in-func-args branch from f7cfa3f to b13f099 Compare July 9, 2020 05:43
… include (for identifiers, numerics) or omit)

Also return the raw value in `getFunctionParameterNames` so as to preserve original quotes when reporting.
@brettz9 brettz9 merged commit 64e1b64 into gajus:master Jul 9, 2020
@michalczaplinski
Copy link
Contributor Author

@brettz9 Hey, sorry about the delay - the quotes I think were a quirk of default formatting with prettier in vscode, my bad!

Thanks for merging in any case! 🙂

@brettz9
Copy link
Collaborator

brettz9 commented Jul 9, 2020

@michalczaplinski : Re: quotes, no worries! I apologize I only later edited in a smile to suggest I wasn't complaining at you. But I'll admit I was nervous there was some new config in our pipeline that was imposing the abomination that is double quotes. :) And no worries re: the delay either. Thanks for the helpful fix!

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.

Does not support destructuring in function arguments if the key in a destructured object is a string
2 participants