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

Support autofix of numerical property access and ternary expressions in no-get rule #1865

Merged
merged 7 commits into from May 18, 2023

Conversation

evanjehl
Copy link
Contributor

@evanjehl evanjehl commented May 18, 2023

This PR is a partial response to this issue (partially fixes #1840) regarding enhancements for the no-get rule, along with another enhancement I propose.

  • Add support for numerical property access this.get(5) ===> this[5] and get(obj, 5) ===> obj[5]
  • Add test cases that prove foo1 === foo2 ? obj.get('bar') : obj.get('baz') ===> foo1 === foo2 ? obj.bar : obj.baz already works as is
  • Add support for ternary expressions that have string/number literals as alternate and consequent (i.e. guaranteed to resolve to a literal) this.get(foo1 === foo2 ? 'foo' : 'bar') ===> foo1 === foo2 ? this.foo : this.bar and get(foo, foo1 === foo2 ? 'bar' : 'baz') ===> foo1 === foo2 ? foo.bar : foo.baz

I attempted to fix cases with variables or other dynamic expressions as the property argument obj.get(foo) ===> obj[foo], but I now recommend that these cases not be autofixed. The reason for this is foo or any other dynamic expression could resolve to a string representation of chained property access (ex. 'bar.baz'), in which case obj[foo] !== obj.get(foo) since obj.get(foo) would resolve at runtime as obj.bar?.baz while obj[foo] would resolve as obj['bar.baz']. Hence, autofixing these cases could cause bugs in existing code.

@@ -399,6 +400,10 @@ function isStringLiteral(node) {
return isLiteral(node) && typeof node.value === 'string';
}

function isNumber(node) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid adding trivial helpers like this per the comment at the top of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed trivial helper

@bmish bmish changed the title Enhancements for no-get Support autofix of numerical property access and ternary expressions in no-get rule May 18, 2023
Copy link
Member

@bmish bmish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Great to improve this autofixer.

@lin-ll lin-ll merged commit a8efed7 into ember-cli:master May 18, 2023
8 checks passed
@evanjehl evanjehl deleted the fix-edge-cases-get branch May 18, 2023 21:30
bmish added a commit to bmish/eslint-plugin-ember that referenced this pull request May 21, 2023
* master: (88 commits)
  Release 11.7.0
  build(deps-dev): Bump @typescript-eslint/parser from 5.59.5 to 5.59.6 (ember-cli#1863)
  Account for only having template tag in `no-empty-glimmer-component-classes` rule (ember-cli#1866)
  Support autofix of numerical property access and ternary expressions in `no-get` rule (ember-cli#1865)
  Release 11.6.0
  build(deps-dev): Bump prettier from 2.8.7 to 2.8.8 (ember-cli#1842)
  build(deps-dev): Bump @typescript-eslint/parser from 5.58.0 to 5.59.5 (ember-cli#1860)
  build(deps-dev): Bump @babel/eslint-parser from 7.21.3 to 7.21.8 (ember-cli#1856)
  build(deps-dev): Bump markdownlint-cli from 0.33.0 to 0.34.0 (ember-cli#1848)
  build(deps-dev): Bump jquery from 3.6.4 to 3.7.0 (ember-cli#1861)
  build(deps-dev): Bump release-it from 15.10.1 to 15.10.3 (ember-cli#1859)
  build(deps): Bump vm2 from 3.9.17 to 3.9.18 (ember-cli#1862)
  Support autofix in gts files (ember-cli#1853)
  Only show `no-undef` errors for templates in gts files (ember-cli#1852)
  Release 11.5.2
  Fix a bug in autofixer and autofix additional cases with `firstObject and `lastObject` in `no-get` rule (ember-cli#1841)
  build(deps-dev): Bump eslint from 8.37.0 to 8.38.0 (ember-cli#1830)
  build(deps-dev): Bump @typescript-eslint/parser from 5.57.0 to 5.58.0 (ember-cli#1837)
  build(deps-dev): Bump typescript from 5.0.3 to 5.0.4 (ember-cli#1832)
  build(deps): Bump vm2 from 3.9.11 to 3.9.17 (ember-cli#1839)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement - no-get rule and auto-fix
3 participants