Fix: Calculate end position of TypeInstantiation (fixes #406)

[duailibe]

When updating prettier to use the 9.0.0 version, I've noticed a comment is moved because the end position of the TypeInstantiation inside a TypeReference is being reported incorrectly.

If you review the two commits separately, you can notice the position change in the snapshot.

This is my first contribution here and I don't actually use TS, so let me know if I did something wrong! Thanks!

[jsf-clabot]

All committers have signed the CLA.

[duailibe]

I believe it's related to #395

cc @JamesHenry

[duailibe]

Should I've created an issue first?

[JamesHenry]

We have just switched over to @eslintbot being a status check, rather than it commenting on the PR directly.

In this case, the feedback is not very explicit in the status check. My guess is the PR title is too long right now.

There is a 72 character limit in the contributing guidelines: https://eslint.org/docs/developer-guide/contributing/pull-requests#step-2-make-your-changes

(But I don't blame you for not spotting that)

I am chatting through with the rest of the team how we can improve the feedback here.

I think the only other point on this PR is getting the ast-alignment-tests to pass. In this case you are using an interface (a construct which is not yet 100% aligned) so you will need to ignore your new spec in the ast-alignment-tests suite.

You should add it into the ignore block here: https://github.com/eslint/typescript-eslint-parser/blob/master/tests/ast-alignment/fixtures-to-test.js#L339

[duailibe]

There is a 72 character limit in the contributing guidelines: https://eslint.org/docs/developer-guide/contributing/pull-requests#step-2-make-your-changes

I read that document but missed somehow this character limit. Really sorry!

I think the only other point on this PR is getting the ast-alignment-tests to pass.

I was already trying to figure out that so I went through the tests and tried other constructs and already had it working with a simple class, so I went with it. (hope it's ok!)

Thank you for the review!!

[JamesHenry]

I was already trying to figure out that so I went through the tests and tried other constructs and already had it working with a simple class, so I went with it. (hope it's ok!)

I'm afraid I don't understand your response. From my comment above, you need to add your spec name into the ignore block here: https://github.com/eslint/typescript-eslint-parser/blob/master/tests/ast-alignment/fixtures-to-test.js#L339

...in order to get the ast-alignment-tests to pass. That is the remaining point blocking your build.

You can run npm run ast-alignment-tests locally to verify

[duailibe]

Sorry I wasn't clear. I simply changed from interface to class since that's unrelated to the bug. Tests pass now locally, but travis is failing because of #407.

As it looks in travis some of the jobs are already running with TS 2.6.2, which is why they fail.

[JamesHenry]

Ah I see!

It seems that's just because SyntaxKind 117 from the relevant TypeScript enum now has two names - it is still the AbstractKeyword, but it is also now the FirstContextualKeyword.

I will open a PR to fix this and we can merge that ahead of yours.

[duailibe]

Thank you! That'll solve both of our issues on upgrading TS version on prettier :)

[JamesHenry]

Great, that's merged. Please go ahead and rebase this against master and force push and it will trigger the travis build to rerun

[duailibe]

Done!

[JamesHenry]

Awesome stuff, thanks a lot!

[duailibe]

Thank you! Do you think we can cut a release with those fixes soon?

[JamesHenry]

I have already cut a release, but we always use commit hashes for typescript-eslint-parser versions in prettier anyway