Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Reasonable and intuitive line counting (proposed fix for #468) #469

Merged
merged 4 commits into from
Jul 25, 2018
Merged

Reasonable and intuitive line counting (proposed fix for #468) #469

merged 4 commits into from
Jul 25, 2018

Conversation

makotom
Copy link
Contributor

@makotom makotom commented Jul 25, 2018

This PR contains:

  1. Fix in a procedure for line counting.
  2. Revisions to a test script in order to verify that the fix works.

I appreciate your attention for review!

@msftclas
Copy link

msftclas commented Jul 25, 2018

CLA assistant check
All CLA requirements met.

package.json Outdated
@@ -40,7 +40,7 @@
"test": "grunt all"
},
"dependencies": {
"tsutils": "^2.12.1"
"tsutils": "^2.12.1 <2.29.0"

Choose a reason for hiding this comment

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

How does [email protected] break test scripts?

We'll need to be able to bump to newer versions of the library, so this is a blocker 😦

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does [email protected] break test scripts?

Need further investigation...

What I know at this moment is that, with v2.29.0, which is released 13 hours ago, tests against the current master also fails, with the similar transpiler errors for "node_modules/tsutils/util/util.d.ts".
// I mean I did not get the error when I tried last night.

Choose a reason for hiding this comment

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

What are the errors you're getting? Sounds like we might need to file a separate issue...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here:

[makoto@kerttu tslint-microsoft-contrib]$ git status
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean
[makoto@kerttu tslint-microsoft-contrib]$ npm i
npm WARN deprecated [email protected]: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue
npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: [email protected] (node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for [email protected]: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})

added 323 packages from 526 contributors and audited 1699 packages in 54.656s
found 1 high severity vulnerability
run `npm audit fix` to fix them, or `npm audit` for details
[makoto@kerttu tslint-microsoft-contrib]$ npm run test

> [email protected] test /home/makoto/tslint-microsoft-contrib
> grunt all

Running "clean:src" (clean) task
>> 1 path cleaned.

Running "copy:json" (copy) task
Copied 3 files

Running "ts:default" (ts) task
Compiling...
Cleared fast compile cache for target: default
Using tsc v2.6.2
node_modules/tsutils/util/util.d.ts(117,73): error TS1005: ';' expected.
node_modules/tsutils/util/util.d.ts(118,1): error TS1128: Declaration or statement expected.
node_modules/tsutils/util/util.d.ts(118,3): error TS1128: Declaration or statement expected.
node_modules/tsutils/util/util.d.ts(119,17): error TS1005: ',' expected.
node_modules/tsutils/util/util.d.ts(119,36): error TS1005: ';' expected.
node_modules/tsutils/util/util.d.ts(119,44): error TS1005: ';' expected.
node_modules/tsutils/util/util.d.ts(120,3): error TS1128: Declaration or statement expected.

>> 7 syntax errors
>> Error: tsc return code: 2
Warning: Task "ts:default" failed. Use --force to continue.

Aborted due to warnings.


Execution Time (2018-07-25 06:57:34 UTC)
clean:src     1.7s  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ 12%
ts:default   11.7s  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ 87%
Total 13.4s

npm ERR! code ELIFECYCLE
npm ERR! errno 3
npm ERR! [email protected] test: `grunt all`
npm ERR! Exit status 3
npm ERR!
npm ERR! Failed at the [email protected] test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/makoto/.npm/_logs/2018-07-25T06_57_47_783Z-debug.log

Choose a reason for hiding this comment

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

Using tsc v2.6.2

Looks like tsutils is depending on syntax features that were added in TypesScript 2.8 and TypeScript 2.9:

export declare type BooleanCompilerOptions = {
    [K in keyof ts.CompilerOptions]: NonNullable<ts.CompilerOptions[K]> extends boolean ? K : never;
} extends {
    [_ in keyof ts.CompilerOptions]: infer U;
} ? U : never;

Specifically, conditional and mapped types. https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript#typescript-28

I'll file issues separately to resolve this. In the meantime, please remove this package change.

You can bump to a local version of TypeScript new enough to support these syntax features without changing package.json using:

npm i [email protected] --no-save

Copy link
Contributor Author

@makotom makotom Jul 25, 2018

Choose a reason for hiding this comment

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

I'll file issues separately to resolve this. In the meantime, please remove this package change.

Sure, I've done it!

@JoshuaKGoldberg
Copy link

Changes look great, thanks! 🎉

I'll separately resolve the tsutils issue, re-run these builds, and merge the PR in if it has no other errors (looks like it shouldn't, from the passing build).

@makotom
Copy link
Contributor Author

makotom commented Jul 25, 2018

Yay, thanks!

I will resume #465 once the merge has completed. 😃

@JoshuaKGoldberg JoshuaKGoldberg merged commit f4c6aae into microsoft:master Jul 25, 2018
@JoshuaKGoldberg JoshuaKGoldberg added this to the 5.1.1 milestone Aug 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants