ni / javascript-styleguide

JavaScript and TypeScript Style Guide
MIT License
9 stars 9 forks source link

ignoreDestructuring override for 'camelcase' rule. #101

Closed mure closed 1 year ago

mure commented 2 years ago

From a discussion in this PR: https://dev.azure.com/ni/DevCentral/_git/Skyline/pullrequest/339229

jattasNI commented 2 years ago

More background: this change would prevent you from having to rename non-camelCase variables that come from destructuring:

// currently disallowed because display_name isn't camel case
return notebook.metadata.parameters.map(({ id, display_name, type }) => {
    const parameter = { id, display_name, type: type as NotebookParameterTypes, value: '', readonly: false };

// current workaround: rename variable before it leaves destructuring
return notebook.metadata.parameters.map(({ id, display_name: displayName, type }) => {
     const parameter = { id, displayName, type: type as NotebookParameterTypes, value: '', readonly: false };
jattasNI commented 2 years ago

Proposal: reject this issue and leave the ruleset as-is.

Rationale: the proposal allows non-camel-case variables to escape beyond the scope of the destructuring, which seems inconsistent. The workaround is pretty simple.

If we agree on this proposal we should update Skyline repo's Workspaces/SystemLinkShared/projects/systemlink-lib-angular/.eslintrc.js to remove its custom configuration of camelcase. It is currently allowing camel_case variables to escape the scope of the destructuring, but nothing takes advantage of it (one case in sl-notebook-execution-form.component.ts does the rename).

jattasNI commented 2 years ago

@mure @TrevorKarjanis @rajsite please 👍 the proposal above to vote for it or submit a counterproposal.

jattasNI commented 2 years ago

offline, @mure pointed out that the docs for the rule disagree with the behavior I saw.

Docs say:

"ignoreDestructuring": true does not check destructured identifiers (but still checks any use of those identifiers later in the code

I saw:

"ignoreDestructuring": true does not check destructured identifiers anywhere. Setting the rule back to the styleguide default doesn't check destructured identifiers within the destructuring but does check use of them later in the code

Carson is going to do his own testing and post back

mure commented 1 year ago

After some investigation and thought, it doesn't feel worth it to bend our generic ruleset to this narrow use case. There's also a TS specific rule that complicates things further, @typescript-eslint/naming-convention. In the over half a year since this issue was filed, we haven't seen it be a problem again. If future projects have to deal with nonstandard names from external APIs for whatever reason, they can handle it locally.