Skip to content

Conversation

@chiubaka
Copy link
Contributor

@chiubaka chiubaka commented Aug 16, 2018

Fix format of TsLint error message extractor.

I'm running with tslint 5.11.0 with default settings for output and my output looks like this:

ERROR: /Users/dchiu/Developer/ets/app/javascript/model/EtsState.ts[1, 10]: Named imports must be alphabetized.
ERROR: /Users/dchiu/Developer/ets/app/javascript/model/EtsState.ts[3, 18]: An interface declaring no members is equivalent to its supertype.
ERROR: /Users/dchiu/Developer/ets/app/javascript/model/EtsState.ts[3, 49]: Unnecessary semicolon
ERROR: /Users/dchiu/Developer/ets/app/javascript/model/EtsState.ts[7, 2]: Missing semicolon
ERROR: /Users/dchiu/Developer/ets/app/javascript/model/EtsState.ts[11, 2]: file should end with a newline
ERROR: /Users/dchiu/Developer/ets/app/javascript/model/index.ts[1, 28]: file should end with a newline
ERROR: /Users/dchiu/Developer/ets/app/javascript/packs/application.tsx[8, 1]: Import sources within a group must be alphabetized.
ERROR: /Users/dchiu/Developer/ets/app/javascript/packs/application.tsx[8, 38]: 'withRouter' is declared but its value is never read.
ERROR: /Users/dchiu/Developer/ets/app/javascript/packs/application.tsx[19, 35]: Missing trailing comma
ERROR: /Users/dchiu/Developer/ets/app/javascript/packs/application.tsx[43, 2]: file should end with a newline
ERROR: /Users/dchiu/Developer/ets/app/javascript/pages/Inbox.tsx[11, 2]: file should end with a newline
ERROR: /Users/dchiu/Developer/ets/app/javascript/reducers/etsReducer.ts[2, 10]: Named imports must be alphabetized.
ERROR: /Users/dchiu/Developer/ets/app/javascript/reducers/etsReducer.ts[8, 4]: file should end with a newline
ERROR: /Users/dchiu/Developer/ets/app/javascript/reducers/index.ts[1, 30]: file should end with a newline
ERROR: /Users/dchiu/Developer/ets/app/javascript/store.ts[1, 38]: Module 'history' is not listed as dependency in package.json
ERROR: /Users/dchiu/Developer/ets/app/javascript/store.ts[3, 10]: Named imports must be alphabetized.
ERROR: /Users/dchiu/Developer/ets/app/javascript/store.ts[7, 10]: Named imports must be alphabetized.
ERROR: /Users/dchiu/Developer/ets/app/javascript/store.ts[7, 10]: 'IEtsState' is declared but its value is never read.
ERROR: /Users/dchiu/Developer/ets/app/javascript/store.ts[15, 2]: Missing semicolon
ERROR: /Users/dchiu/Developer/ets/app/javascript/store.ts[26, 3]: file should end with a newline

The regex for the message extractor looks like it may be missing the "ERROR: "? As is I get this output when running overcommit -r:

Unexpected output: unable to determine line number or type of error/warning for output:

ERROR: app/javascript/model/EtsState.ts[1, 10]: Named imports must be alphabetized.
ERROR: app/javascript/model/EtsState.ts[3, 18]: An interface declaring no members is equivalent to its supertype.
ERROR: app/javascript/model/EtsState.ts[3, 49]: Unnecessary semicolon
ERROR: app/javascript/model/EtsState.ts[7, 2]: Missing semicolon
ERROR: app/javascript/model/EtsState.ts[11, 2]: file should end with a newline
ERROR: app/javascript/model/index.ts[1, 28]: file should end with a newline
ERROR: app/javascript/reducers/etsReducer.ts[2, 10]: Named imports must be alphabetized.
ERROR: app/javascript/reducers/etsReducer.ts[8, 4]: file should end with a newline
ERROR: app/javascript/reducers/index.ts[1, 30]: file should end with a newline
ERROR: app/javascript/store.ts[1, 38]: Module 'history' is not listed as dependency in package.json
ERROR: app/javascript/store.ts[3, 10]: Named imports must be alphabetized.
ERROR: app/javascript/store.ts[7, 10]: Named imports must be alphabetized.
ERROR: app/javascript/store.ts[15, 2]: Missing semicolon
ERROR: app/javascript/store.ts[26, 3]: file should end with a newline

✗ One or more pre-commit hooks failed

Which is still useful, but seems like overcommit is complaining that it's confused about the message format.

Fix format of TsLint error message extractor.
@sds
Copy link
Owner

sds commented Aug 17, 2018

Thanks for the PR, @chiubaka!

I'm not familiar with tslint so I'll defer to the original author of the hook, @jhuiting, to weigh in here.

extract_messages(
output.split("\n"),
/^(?<file>.+?(?=\[))[^\d]+(?<line>\d+).*?/
/^ERROR: (?<file>.+?(?=\[))[^\d]+(?<line>\d+).*?/
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this added here, the TSLint output like the comment on line 11 so this will never match? If you're aiming at changing the message that we return, that's possible but that should happen outside of the regex.

@jhuiting
Copy link
Contributor

@chiubaka @sds Left a review comment

@sds
Copy link
Owner

sds commented Oct 8, 2018

@chiubaka any response to @jhuiting's comments? With the tests failing we won't be merging this anyway. Would be good to know that ERROR is indeed necessary, or not the result of some other preprocessor (which I can't validate as I don't use tslint myself).

@davidgengenbach
Copy link
Contributor

I created a PR with a regexp which supports both the old as well as the new output format: #601

@sds
Copy link
Owner

sds commented Oct 17, 2018

Fixed in #601

@sds sds closed this Oct 17, 2018
@sds sds added the bug label Oct 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants