Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add
prefer-class-fields
rule #2512base: main
Are you sure you want to change the base?
Add
prefer-class-fields
rule #2512Changes from 2 commits
5d25bd3
bb42087
cd95420
e5e486d
14025db
21e3cf1
110881d
378f58b
633829c
0e6afb3
a7edb37
3a9c5f8
f9caf1b
3376845
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tab-indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
378f58b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The operator didn't checked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it should be ignored. Fixed here: 21e3cf1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The left property didn't checked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in this case we should ignore any kind of reporting -
foo.bar
might be dynamic and checking if it is or not is a rabbit hole with potentially lots of nested jumps.I can handle cases like
this['foo'] = 'foo';
thoughThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a valid test case to cover the dynamic case: 633829c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here: 0e6afb3 added support for simple strings & template strings.I ended up removing the support for strings/template strings. With support for multiple "name formats" the whole process looks like this:
foo='foo'
,['foo']='foo'
, etc.foo='foo'
Supporting those IMO brings lots of unespected edge cases in the points 1 & 2. It's hard to predict which string and how can be represented. Simple
this.foo
can be written asthis['foo']
,this["foo"]
orthis[`foo`]
, but string with kebab-case like: "foo-something" can be only used asthis['foo-something']
,this["foo-something"]
orthis[`foo-something`]
. For every of those "name formats" there are other characters that are restricted (and I don't want even start about the escaped characters 😄 ).It's just too much of a hassle and places that can potentially break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't mean to ask support for strings, please test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't worry, I didn't understood it well then :)
Done: f9caf1b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix also changes code if the property already exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. IMO in this case the output should be:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done: a7edb37