Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Fix #4530: no-string-throw fixer creates syntax errors #4540

Merged
merged 8 commits into from
Mar 31, 2019
Merged

Fix #4530: no-string-throw fixer creates syntax errors #4540

merged 8 commits into from
Mar 31, 2019

Conversation

rrogowski
Copy link
Contributor

PR checklist

Overview of change:

Insert whitespace after throw keyword (if not present) when fixing no-string-throw.

Input

throw('component requires CSS height property');

Fix (Before)

thrownew Error(('component requires CSS height property'));

Fix (After)

throw new Error(('component requires CSS height property'));

Is there anything you'd like reviewers to focus on?

  • The double parentheses behavior is unrelated to this change, and existed before this bugfix.
    • I am happy to create an issue for this if so desired.

CHANGELOG.md entry:

[bugfix] no-string-throw fix inserts whitespace if not present after throw keyword

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Good start! Just a bit of cleanup to do the same thing.

test/rules/no-string-throw/test.ts.lint Show resolved Hide resolved
src/rules/noStringThrowRule.ts Outdated Show resolved Hide resolved
@rrogowski
Copy link
Contributor Author

@JoshuaKGoldberg finished those changes you requested.

Oddly enough, I'm seeing that my implementation is failing on CircleCI, but all tests are passing locally on my end. I suspect it has to do with the fact that a Replacement fix is applied to the same position (expression.getStart). Any thoughts as to why there might be this disparity?

@JoshuaKGoldberg
Copy link
Contributor

Very spooky 👻... Have you tried installing a version of TypeScript that's failing in CI, such as 2.7 or 3.0? Perhaps .pos instead of .getStart() might help?

Alternately, you could try reordering the lines? The TSLint application of fixes is known to be not great at overlapping fixes, but that doesn't seem like it should be affecting these?

@rrogowski
Copy link
Contributor Author

I have tried all versions of TypeScript that are failing on CI: 2.1, 2.4, 2.7, 2.8, 2.9, 3.0, rc, and next. All tests are all passing locally with each of these TypeScript versions installed, yet the only version passing on CI is next. Using .pos instead of .getStart did not remedy the situation either. I will continue to investigate this, but I am a bit stumped...

@rrogowski
Copy link
Contributor Author

rrogowski commented Mar 5, 2019

To clarify, my workflow is this.

  1. Checkout to fresh version of this branch
  2. yarn
  3. yarn compile
  4. yarn test (all tests pass)
  5. yarn add typescript@2.1
  6. yarn test (again, all tests pass)
  7. Repeat steps 5 & 6 for all failing TypeScript versions

Am I missing something?

EDIT: I was missing one crucial step, which was installing the correct node version for each test. For example, the test for typescript@2.1 uses node 6. The tests are now failing locally, and I can debug this issue further.

@rrogowski
Copy link
Contributor Author

@JoshuaKGoldberg ready for your pass.

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

🚀 Looks great, thanks @rrogowski!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants