Skip to content
This repository has been archived by the owner. It is now read-only.

[Bugfix] Fix for whitespace rule requiring extra space in ImportType in TS 2.9 #3992

Closed
wants to merge 1 commit into from

Conversation

Tenga
Copy link

@Tenga Tenga commented Jun 24, 2018

PR checklist

  • Addresses an existing issue: #3987
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

Overview of change:

Fixes the bug where the new ImportType syntax required extra spacing when whitespace rule was enabled.

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

  • Beware, my first dive into TSLint and the AST. 馃槃
  • Might have overdone or missed the test cases
  • As this seems to be only relevant in TS 2.9 and above, not sure how you usually deal with cases like this if you want to support older TS versions?
    • I've done the first thing that came to mind and typecasted the SyntaxKind as any and let the ImportKind end up undefined to fail the check.
    • Is there an standard way on how this is handled usually?
    • I've extracted the tests for this to a file which is only ran on TS 2.9, I'm assuming that's fine as the syntax for this to work should get struck down by the TS compiler for users of the older TS anyway?

CHANGELOG.md entry:

[bugfix] whitespace rule no longer requires extra spaces with the TS 2.9 import type syntax

@@ -296,6 +296,10 @@ function walk(ctx: Lint.WalkContext<Options>) {
}
break;
case ts.SyntaxKind.ImportKeyword:
if (parent.kind === (ts.SyntaxKind as any).ImportType) {
Copy link
Contributor

@suchanlee suchanlee Jun 28, 2018

Choose a reason for hiding this comment

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

can you first check if ts.SyntaxKind.ImportType is defined?

Copy link
Author

@Tenga Tenga Jun 28, 2018

Choose a reason for hiding this comment

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

Sure, done. Tho it seems redundant to me in this case, hence why it wasn't there in the first place. 馃槃

@johnwiseheart
Copy link
Contributor

@johnwiseheart johnwiseheart commented Jul 16, 2018

Hi @Tenga, just wanted to check if you have signed the CLA as that appears to be the remaining test left to pass. The cla-bot has been a little confused lately, so if you're unsure what the CLA is, I've copied its message from another PR:

Thanks for your interest in palantir/tslint, @Tenga! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@Tenga
Copy link
Author

@Tenga Tenga commented Jul 16, 2018

Hey @johnwiseheart. I'm aware of it, I'm just waiting for the go-ahead from my employer before signing. 鈱氾笍

@johnwiseheart johnwiseheart added the PR: waiting for CLA label Jul 18, 2018
@Tenga
Copy link
Author

@Tenga Tenga commented Jul 23, 2018

@johnwiseheart CLA has been signed.

@@ -296,6 +296,10 @@ function walk(ctx: Lint.WalkContext<Options>) {
}
break;
case ts.SyntaxKind.ImportKeyword:
if ((ts.SyntaxKind as any).ImportType !== undefined && parent.kind === (ts.SyntaxKind as any).ImportType) {
Copy link
Contributor

@johnwiseheart johnwiseheart Jul 25, 2018

Choose a reason for hiding this comment

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

Hmm, I don't like having to cast as any. What is the issue here - is it that ts.SyntaxKind.ImportType is a ts2.9+ thing?

Copy link
Author

@Tenga Tenga Jul 25, 2018

Choose a reason for hiding this comment

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

Yup.

Copy link
Contributor

@johnwiseheart johnwiseheart Jul 25, 2018

Choose a reason for hiding this comment

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

@giladgray @suchanlee @jkillian any ideas on how to work around this?

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg Nov 7, 2018

Choose a reason for hiding this comment

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

Let's define a shared isImportKeyword in src/utils.ts similar to existing helper functions from tsutils.

@johnwiseheart johnwiseheart removed the PR: waiting for CLA label Jul 25, 2018
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Code generally looks good; just one comment on the isImportKeyword function.

@@ -296,6 +296,10 @@ function walk(ctx: Lint.WalkContext<Options>) {
}
break;
case ts.SyntaxKind.ImportKeyword:
if ((ts.SyntaxKind as any).ImportType !== undefined && parent.kind === (ts.SyntaxKind as any).ImportType) {
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg Nov 7, 2018

Choose a reason for hiding this comment

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

Let's define a shared isImportKeyword in src/utils.ts similar to existing helper functions from tsutils.

@JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Nov 7, 2018

Looks like #4243 is the same as this PR but with the comment resolved (and great tests reflected). Thanks for sending this in @Tenga! Closing this now that #4243 went in, but still very much appreciated!

@Tenga
Copy link
Author

@Tenga Tenga commented Nov 7, 2018

While I'm happy this fairly thin PR will finally be resolved, it leaves a bad taste in my mouth.

This PR has been, with my full understanding, sitting in the queue for months, with me waiting for feedback/advice on how you guys would like to resolve a style problem. The end result is merging a PR that openly rips 90% of the work from this PR, because it was hard to wait a couple of hours to amend this PR with a one-liner change?

炉\_(銉)_/炉

@JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Nov 7, 2018

@Tenga Thanks for the feedback. You're right, and I'm sorry for marginalizing your contribution here. I should have waited a few days before merging in the other PR.

For context on the sitting in the queue for months, TSLint hasn't had an active maintainer in those months, and I'm coming in now as a new maintainer to clean up the issues and PR queues. My goal here was to minimize the amount of pending work we have, but I went a bit overboard and didn't stop to think about this.

If you send another PR, I can promise the same thing won't happen twice.

@Tenga
Copy link
Author

@Tenga Tenga commented Nov 7, 2018

@JoshuaKGoldberg Appreciate the response. 馃憤

If I encounter an opportunity to contribute again, I'll certainly do so.

Just to clarify as I probably should've probably phrased it better. I understand that OSS is hard. "Sitting in queue for months" was not meant to be a diss or dissatisfaction with that, just the understanding of the reality. (Contributing guidelines prepare you for that anyway. 馃槢)

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.

None yet

4 participants