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

[prefer-immutable-types] Doesn't take into consideration ignoreNamePattern with hash symbol #910

Closed
Clarchik opened this issue Dec 28, 2024 · 4 comments · Fixed by #921
Closed
Labels
Breaking Change This change will require a new major release. Status: Released It's now live. Type: Bug Inconsistencies or issues which will cause a problem for users or implementors.

Comments

@Clarchik
Copy link

Bug Report

Seems like rule does not work as intended.

Expected behavior

Rule below should not produce error for variables which follows # symbol (TS private modifier)

'functional/prefer-immutable-types': [
      'error',
      {
        ignoreNamePattern: ['_', '#']
      }
    ],

Actual behavior

Works for

_anyName variable

But produces error for

#anyName variable

Property should have a readonly modifier eslint [functional/prefer-immutable-types]

Steps to reproduce

Latest version (7.2.0) with the rule snippet above

Proposed changes

@Clarchik Clarchik added Status: Triage This issue needs to be triaged. Type: Bug Inconsistencies or issues which will cause a problem for users or implementors. labels Dec 28, 2024
@RebeccaStevens
Copy link
Collaborator

So the # symbol at the start of a private field's name isn't internally considered part of the name itself.
So ignoreNamePattern: "^mutable" would ignore class Klass { #mutableFoo: string[] = []; }

I could change this in the future to require a # for private fields, but this would be a breaking change.

You could use the ignoreClasses: "fieldsOnly" option, but this will ignore all class fields, not just private ones. Is this suitable for your needs?

@RebeccaStevens RebeccaStevens added Resolution: By Design The behavior reported in the issue is actually correct. Status: Awaiting Response Issue or PR awaits response from the creator. and removed Type: Bug Inconsistencies or issues which will cause a problem for users or implementors. Status: Triage This issue needs to be triaged. labels Jan 5, 2025
@Clarchik
Copy link
Author

Clarchik commented Jan 5, 2025

@RebeccaStevens

You could use the ignoreClasses: "fieldsOnly" option, but this will ignore all class fields, not just private ones. Is this suitable for your needs?

Not sure, need to test it.

I was playing around source code of the lib and found the solution for myself which works, but the question is - is it a breaking change?

Changing this

if (isIdentifier(node) || isPrivateIdentifier(node)) {
 mut_identifierText = node.name;
}

to this

if (isIdentifier(node)) {
 mut_identifierText = node.name;
} else if(isPrivateIdentifier(node)) {
 mut_identifierText = `#${node.name}`;
} 

I found that only hash symbol is recognized as PrivateIdentifier and adding # before var name - makes sense in my case.
What's your opinion on that?

RebeccaStevens added a commit that referenced this issue Jan 14, 2025
BREAKING CHANGE: when using patterns to match against PrivateIdentifiers, their names are now
prefixed with an #.

fix #910 fix#911
@RebeccaStevens RebeccaStevens added Type: Bug Inconsistencies or issues which will cause a problem for users or implementors. Breaking Change This change will require a new major release. and removed Status: Awaiting Response Issue or PR awaits response from the creator. Resolution: By Design The behavior reported in the issue is actually correct. labels Jan 14, 2025
@RebeccaStevens
Copy link
Collaborator

After thinking about this for awhile, I believe that yes, that should be the desired behavior. It is a breaking change though.

RebeccaStevens added a commit that referenced this issue Jan 14, 2025
BREAKING CHANGE: when using patterns to match against PrivateIdentifiers, their names are now
prefixed with a #.

fix #910 fix #911
@github-actions github-actions bot added the Status: Released It's now live. label Jan 14, 2025
RebeccaStevens added a commit that referenced this issue Jan 14, 2025
BREAKING CHANGE: when using patterns to match against PrivateIdentifiers, their names are now prefixed with a #.

fix #910 fix #911
github-actions bot pushed a commit that referenced this issue Jan 14, 2025
# [8.0.0](v7.3.0...v8.0.0) (2025-01-14)

### Bug Fixes

* prefix PrivateIdentifier names with a # ([#921](#921)) ([09b6474](09b6474)), closes [#910](#910) [#911](#911)

### BREAKING CHANGES

* when using patterns to match against PrivateIdentifiers, their names are now prefixed with a #.
Copy link

🎉 This issue has been resolved in version 8.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@eslint-functional eslint-functional deleted a comment from github-actions bot Jan 14, 2025
renovate bot added a commit to mmkal/eslint-plugin-mmkal that referenced this issue Jan 14, 2025
renovate bot added a commit to mmkal/eslint-plugin-mmkal that referenced this issue Jan 15, 2025
renovate bot added a commit to mmkal/eslint-plugin-mmkal that referenced this issue Jan 15, 2025
renovate bot added a commit to mmkal/eslint-plugin-mmkal that referenced this issue Jan 15, 2025
renovate bot added a commit to mmkal/eslint-plugin-mmkal that referenced this issue Jan 15, 2025
renovate bot added a commit to mmkal/eslint-plugin-mmkal that referenced this issue Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change This change will require a new major release. Status: Released It's now live. Type: Bug Inconsistencies or issues which will cause a problem for users or implementors.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants