feat(eslint-plugin): removed value from abstract property nodes #3765

JoshuaKGoldberg · 2021-08-21T17:24:16Z

Continues #3606 by removing the value for TSAbstractClassProperty nodes.

There are a few places that check for that member so I figured it's probably reasonable to go the extra mile.

typescript-eslint · 2021-08-21T17:24:19Z

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

nx-cloud · 2021-08-21T17:26:17Z

Nx Cloud Report

CI ran the following commands for commit 6f1c9fd. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status	Command
	`nx test @typescript-eslint/eslint-plugin-internal`
	`nx test @typescript-eslint/eslint-plugin-tslint`
	`nx test @typescript-eslint/eslint-plugin-tslint`
	`nx test @typescript-eslint/eslint-plugin`
	`nx test @typescript-eslint/eslint-plugin`
	`nx test @typescript-eslint/eslint-plugin-tslint`
	`nx test @typescript-eslint/eslint-plugin`
	`nx test @typescript-eslint/eslint-plugin-tslint`
	`nx test @typescript-eslint/eslint-plugin`
	`nx test @typescript-eslint/parser`
	`nx test @typescript-eslint/parser`
	`nx test @typescript-eslint/parser`
	`nx test @typescript-eslint/experimental-utils`
	`nx test @typescript-eslint/experimental-utils`
	`nx test @typescript-eslint/scope-manager`
	`nx test @typescript-eslint/experimental-utils`
	`nx test @typescript-eslint/scope-manager`
	`nx test @typescript-eslint/parser`
	`nx test @typescript-eslint/scope-manager`
	`nx test @typescript-eslint/experimental-utils`

Sent with 💌 from NxCloud.

Bumps [@types/jest](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/jest) from 26.0.24 to 27.0.1. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/jest) --- updated-dependencies: - dependency-name: "@types/jest" dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

codecov · 2021-08-22T17:50:55Z

Codecov Report

Merging #3765 (6f1c9fd) into v5 (e80056f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##               v5    #3765   +/-   ##
=======================================
  Coverage   92.57%   92.58%           
=======================================
  Files         325      327    +2     
  Lines       11283    11294   +11     
  Branches     3181     3183    +2     
=======================================
+ Hits        10445    10456   +11     
  Misses        384      384           
  Partials      454      454

Flag	Coverage Δ
unittest	`92.58% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...plugin/src/rules/explicit-module-boundary-types.ts	`89.47% <ø> (ø)`
packages/typescript-estree/src/convert.ts	`98.28% <ø> (+<0.01%)`	⬆️
packages/visitor-keys/src/visitor-keys.ts	`100.00% <ø> (ø)`
...ackages/eslint-plugin/src/rules/member-ordering.ts	`93.12% <100.00%> (+0.05%)`	⬆️
...kages/scope-manager/src/referencer/ClassVisitor.ts	`92.03% <100.00%> (+0.07%)`	⬆️
packages/eslint-plugin/src/rules/semi.ts	`92.85% <0.00%> (ø)`
packages/eslint-plugin/src/rules/indent.ts	`92.85% <0.00%> (ø)`
packages/eslint-plugin/src/rules/quotes.ts	`89.47% <0.00%> (ø)`
packages/eslint-plugin/src/rules/typedef.ts	`96.05% <0.00%> (ø)`
packages/typescript-estree/src/node-utils.ts	`97.83% <0.00%> (ø)`
... and 122 more

bradzacher

Hmm - I wonder - instead of removing the property altogether, do we instead want to just set value: null?
I'm trying to figure out if it's better to make the AST nodes different shapes or not.

We will want to switch ClassProperty to PropertyDefinition to match the ESTree spec for the next version.
So it stands to reason that we'd also rename TSAbstractClassProperty to TSAbstractPropertyDefinition to match.

However - I wonder - do we want to keep that node still?
Or do we want to just add a modifier boolean on to PropertyDefinition?

I'm not entirely sure which is the 100% correct thing to do.
I'm guessing that it's probably better to keep them split as it means it's trivial for existing rules to not false-positive match on them.

Hmmmmmmmmmmm.

I think for now - what we should do is leave the value property on TSAbstractClassProperty and just set it to be "always null". The benefit of this change is that you can iterate through the similar nodes (props and abstract props) without doing if (node.type === 'ClassProperty') { // use node.value }

bradzacher

two small comments - otherwise lgtm - thanks for doing this!

packages/eslint-plugin/src/rules/member-ordering.ts

packages/typescript-estree/src/convert.ts

JoshuaKGoldberg · 2021-08-29T22:57:31Z

Ah, thanks - I didn't end up having time to get to the changes this week. Much appreciated!

* chore: bump @types/jest from 26.0.24 to 27.0.1 (#3748) Bumps [@types/jest](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/jest) from 26.0.24 to 27.0.1. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/jest) --- updated-dependencies: - dependency-name: "@types/jest" dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore: address comments Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Brad Zacher <brad.zacher@gmail.com>

JoshuaKGoldberg force-pushed the abstract-property-no-initializer branch from 03e02ce to 8354f15 Compare August 21, 2021 17:34

bradzacher added this to the 5.0.0 milestone Aug 21, 2021

bradzacher added the AST PRs and Issues about the AST structure label Aug 21, 2021

JoshuaKGoldberg force-pushed the abstract-property-no-initializer branch from 8354f15 to 72dd026 Compare August 21, 2021 17:58

bradzacher changed the base branch from master to v5 August 21, 2021 22:12

bradzacher added the breaking change This change will require a new major version to be released label Aug 21, 2021

JoshuaKGoldberg force-pushed the abstract-property-no-initializer branch from 72dd026 to 714266f Compare August 22, 2021 17:35

Merge branch 'v5' into abstract-property-no-initializer

95ff08c

JoshuaKGoldberg marked this pull request as ready for review August 22, 2021 17:59

bradzacher mentioned this pull request Aug 23, 2021

TypeScript 4.4 Syntax Support #3606

Closed

bradzacher reviewed Aug 23, 2021

View reviewed changes

bradzacher requested changes Aug 28, 2021

View reviewed changes

packages/eslint-plugin/src/rules/member-ordering.ts Outdated Show resolved Hide resolved

packages/typescript-estree/src/convert.ts Outdated Show resolved Hide resolved

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Aug 28, 2021

chore: address comments

6f1c9fd

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Aug 28, 2021

bradzacher approved these changes Aug 28, 2021

View reviewed changes

bradzacher merged commit 69aebba into typescript-eslint:v5 Aug 28, 2021

JoshuaKGoldberg deleted the abstract-property-no-initializer branch August 29, 2021 22:57

github-actions bot locked as resolved and limited conversation to collaborators Sep 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): removed value from abstract property nodes #3765

feat(eslint-plugin): removed value from abstract property nodes #3765

JoshuaKGoldberg commented Aug 21, 2021

typescript-eslint bot commented Aug 21, 2021

nx-cloud bot commented Aug 21, 2021 •

edited

Loading

codecov bot commented Aug 22, 2021 •

edited

Loading

bradzacher left a comment

bradzacher left a comment

JoshuaKGoldberg commented Aug 29, 2021

feat(eslint-plugin): removed value from abstract property nodes #3765

feat(eslint-plugin): removed value from abstract property nodes #3765

Conversation

JoshuaKGoldberg commented Aug 21, 2021

typescript-eslint bot commented Aug 21, 2021

nx-cloud bot commented Aug 21, 2021 • edited Loading

Nx Cloud Report

codecov bot commented Aug 22, 2021 • edited Loading

Codecov Report

bradzacher left a comment

Choose a reason for hiding this comment

bradzacher left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Aug 29, 2021

nx-cloud bot commented Aug 21, 2021 •

edited

Loading

codecov bot commented Aug 22, 2021 •

edited

Loading