New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding typescript-tslint #949

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@Simplify
Member

Simplify commented Apr 20, 2016

As described in #947.

Edit: Waffle, connects to #947 (@lunaryorn)

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Apr 20, 2016

@Simplify Nice, thank you very much. I'll review and probably leave a couple of remarks.

Generally, I'd like to ask you to squash all commits into a single one, and rephrase the commit message according to our commit guidelines.

.. option:: flycheck-typescript-tslint-rulesdir
Additional rules directory, for user created rules. For more details please
check `TSLint CLI usage <http://palantir.github.io/tslint/usage/cli/>`_.

This comment has been minimized.

@lunaryorn

lunaryorn Apr 20, 2016

Contributor

check -> see.

This comment has been minimized.

@lunaryorn

lunaryorn Apr 20, 2016

Contributor

Also, please indent .. option:: text with three spaces relative to .. option::. I think—again it's hard to see on Github—you're only using two spaces here.

.. syntax-checker:: typescript-tslint
Check style and syntax with `TSLint <https://github.com/palantir/tslint>`_.

This comment has been minimized.

@lunaryorn

lunaryorn Apr 20, 2016

Contributor

Check syntax and style […] :)

flycheck.el Outdated
@@ -8456,6 +8457,42 @@ See URL `http://www.gnu.org/software/texinfo/'."
line-end))
:modes texinfo-mode)
(flycheck-def-config-file-var flycheck-typescript-tslint-config typescript-tslint "tslint.json"
:safe #'stringp
:package-version '(flycheck . "0.26"))

This comment has been minimized.

@lunaryorn

lunaryorn Apr 20, 2016

Contributor

0.26 -> 26, please.

flycheck.el Outdated
`flycheck-typescript-tslint-config' is nil or refers to a non-existing file.
See URL
`https://github.com/palantir/tslint'."

This comment has been minimized.

@lunaryorn

lunaryorn Apr 20, 2016

Contributor

Please re-wrap this entire docstring with M-q. The current wrapping looks a bit strange.

flycheck.el Outdated
:type '(choice (const :tag "No custom rules directory" nil)
(directory :tag "Custom rules directory"))
:safe #'stringp
:package-version '(flycheck . "0.26"))

This comment has been minimized.

@lunaryorn

lunaryorn Apr 20, 2016

Contributor

Likewise 26, here :)

flycheck.el Outdated
(option "--rules-dir" flycheck-typescript-tslint-rulesdir)
source)
:error-patterns
((warning line-start (one-or-more not-newline) "[" line ", " column "]: " (message) line-end))

This comment has been minimized.

@lunaryorn

lunaryorn Apr 20, 2016

Contributor

What's in the (one-or-more not-newline) group?

By the way, I noticed that tslint can output JSON. Would it be possible to write a custom :error-parser for this JSON format? Generally, we prefer to parse structured output formats from syntax checkers instead of the human-readable error output, because in our experience the structured formats are a lot less fragile and don't change very often, which makes maintenance much easier for us on the long run.

@@ -4283,6 +4283,13 @@ Why not:
'(9 nil warning "printindex before document beginning: @printindex cp"
:checker texinfo)))
(flycheck-ert-def-checker-test typescript-tslint typescript nil
(flycheck-ert-should-syntax-check

This comment has been minimized.

@lunaryorn

lunaryorn Apr 20, 2016

Contributor

Please fix the indentation here. Look at how the other tests are indented.

To each Emacs the indentation rules for flycheck-ert-def-checker-test, do M-x load-library RET flycheck-ret before editing this file.

@@ -0,0 +1,94 @@
{

This comment has been minimized.

@lunaryorn

lunaryorn Apr 20, 2016

Contributor

Would mind to strip this configuration file down to the bare minimum? It's rather long currently, and for us who don't know typescript it's very hard to understand which of these rules are actually required for the test case and must be edited with care.

A configuration file is mandatory for this syntax checker. If
`flycheck-typescript-tslint-config` is not set or the configuration
file not found this syntax checker will not be applied.

This comment has been minimized.

@lunaryorn

lunaryorn Apr 20, 2016

Contributor

I think the indentation of this line is off, isn't it? Hard to see on Github.

@Simplify

This comment has been minimized.

Member

Simplify commented Apr 20, 2016

Thanks for fast reply :)
I guess that we can ignore travis-ci 24.3 error?

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Apr 20, 2016

@Simplify Yes, sure. I've taken a look, looks like a temporary glitch.

@Simplify

This comment has been minimized.

Member

Simplify commented Apr 20, 2016

Are you sure that you prefer JSON output? Default output for TSLint is 'prose', that I use here. JSON output doesn't provide any additional information and it's longer:

$ tslint --format json sample.ts 
[{"endPosition":{"character":25,"line":0,"position":25},"failure":"unused variable: 'invalidAlignment'","name":"sample.ts","ruleName":"no-unused-variable","startPosition":{"character":9,"line":0,"position":9}},{"endPosition":{"character":7,"line":1,"position":37},"failure":"unused variable: 'a'","name":"sample.ts","ruleName":"no-unused-variable","startPosition":{"character":6,"line":1,"position":36}},{"endPosition":{"character":14,"line":2,"position":76},"failure":"missing semicolon","name":"sample.ts","ruleName":"semicolon","startPosition":{"character":14,"line":2,"position":76}}]
$ tslint sample.ts 
sample.ts[1, 10]: unused variable: 'invalidAlignment'
sample.ts[2, 7]: unused variable: 'a'
sample.ts[3, 15]: missing semicolon

JSON output is mostly used by task runners like Gulp.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Apr 20, 2016

@Simplify I wouldn't have asked if I weren't sure 😉 I'm aware that JSON is longer and doesn't contain any additional information, but unlike prose it follows strict syntactic rules so it's much less likely to break.

@Simplify

This comment has been minimized.

Member

Simplify commented Apr 20, 2016

Ok, I'll do JSON parsing. You want me to rebase all commits into single one?

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Apr 20, 2016

@Simplify Yes, please do that ☺️

@Simplify

This comment has been minimized.

Member

Simplify commented Apr 20, 2016

Ok, JSON parser is added. Can you check code again? I'll merge commits tomorrow.

@Simplify

This comment has been minimized.

Member

Simplify commented Apr 21, 2016

Ok, all commits are merged into single one.

@Simplify

This comment has been minimized.

Member

Simplify commented May 4, 2016

@lunaryorn when can I expect code review?

flycheck.el Outdated
:checker checker
:buffer buffer
:filename (plist-get emessage :name)) errors)))
(nreverse errors))))

This comment has been minimized.

@lunaryorn

lunaryorn May 4, 2016

Contributor

Is there a reason why you're using plists as the output type here? I think it'd be easier to read if you used alists in combination with let-alist. Take a look at the existing XML error parsers which make use of let-alist for an example.

flycheck.el Outdated
(let ((json-object-type 'plist))
(let ((errors)
(tslint-json-output (json-read-from-string output)))
(dotimes (n (length tslint-json-output))

This comment has been minimized.

@lunaryorn

lunaryorn May 4, 2016

Contributor

Why do you use dotimes? Can't you use dolist here?

flycheck.el Outdated
about TSLint."
(let ((json-object-type 'plist))
(let ((errors)
(tslint-json-output (json-read-from-string output)))

This comment has been minimized.

@lunaryorn

lunaryorn May 4, 2016

Contributor

The indentation looks a little strange to me. Are there tabs in this file? Can you try to reindent the entire function?

@@ -3023,6 +3023,36 @@ of the file will be interrupted because there are too many #ifdef configurations
:buffer 'buffer
:filename "foo.php")))))
(defconst flycheck-typescript-tslint-json

This comment has been minimized.

@lunaryorn

lunaryorn May 4, 2016

Contributor

Would you mind to port this to a buttercup test? Take a look at test/specs, we already have a file for Haskell there, and you'd create a similar one for Typescript. There's also an existing matcher already called :to-be-equal-flycheck-errors which compares to list of errors specifically, with much better error reporting than ert matchers.

We only use ERT for syntax checker integration tests, i.e. the other one you've written.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented May 4, 2016

@Simplify I'm sorry I didn't have much time, there's been a lot of work and travel in the last two weeks.

I've given it a cursory review, and left a couple of comments. Please take a look. I'll also review in more detail later, but please be patient.

@Simplify

This comment has been minimized.

Member

Simplify commented May 5, 2016

@lunaryorn Changes are implemented. When you have time, please review it again. It helps me a lot. Also, I'm curious what are your indentation settings for buttercup test files and how you apply it only to those files only. My Emacs springs (it after (describe, not two spaces as in your Haskell test file. Had to change everything almost line by line.

(describe "TSLint checker"
(it "should parse example TSLint JSON output"
(let ((example-json-output "[{\"endPosition\":{\"character\":25,\"line\":0,\"position\":25},

This comment has been minimized.

@lunaryorn

lunaryorn May 5, 2016

Contributor

Please do me a favour and move the whole definition in a defconst, e.g. (defconst flycheck/typescript-lint-json …) and use this constant here. That's just a lot easier to read when the JSON string doesn't obscure the whole test.

\"ruleName\":\"semicolon\",
\"startPosition\":{\"character\":14,\"line\":2,\"position\":76}}]"))
(expect (flycheck-parse-tslint example-json-output 'checker 'buffer)
:to-be-equal-flycheck-errors (list

This comment has been minimized.

@lunaryorn

lunaryorn May 5, 2016

Contributor

Please wrap before list so that the whole expression sits below :to-equal-…. Again that's easier to read :)

(flycheck-ert-def-checker-test typescript-tslint typescript nil
(flycheck-ert-should-syntax-check
"language/typescript/sample.ts" 'typescript-mode
'(1 10 warning "unused variable: 'invalidAlignment'" :checker typescript-tslint :id "no-unused-variable")

This comment has been minimized.

@lunaryorn

lunaryorn May 5, 2016

Contributor

Please wrap lines so that they're shorter than 80 characters.

flycheck.el Outdated
@@ -8456,6 +8481,42 @@ See URL `http://www.gnu.org/software/texinfo/'."
line-end))
:modes texinfo-mode)
(flycheck-def-config-file-var flycheck-typescript-tslint-config typescript-tslint "tslint.json"

This comment has been minimized.

@lunaryorn

lunaryorn May 5, 2016

Contributor

Please wrap lines so that they're shorter than 80 characters.

GH-947 - TSLint checker for typescript-mode
Added typescript-mode to Cask file
Added typescript-tslint checker
Added documentation for typescript-tslint
Added integration tests for typescript-tslint
Added ERT tests for flycheck-tslint-parse
@Simplify

This comment has been minimized.

Member

Simplify commented May 6, 2016

@lunaryorn No, TSLint does not provide severity info. There is open issue for that with label "Needs proposal". My opinion is that almost all syntax and style output info from TSLint are warnings. TypeScript is superset of JavaScript (+ types and some ES6 features), and any JavaScript code is valid TypeScript code. Having bunch of warnings about missing semicolons, not providing variable types or something similar will not stop TypeScript "compiler" (tsc) to generate valid JavaScript in most cases.
Real errors are generated from TypeScript compiler. There is tide-mode package that use patched tsserver (normally part of typescript node package) and hooks into flycheck with compiler errors and warnings (also uses company-mode for code completion). There is another similar elisp package but targeted at ac and flymake users.

Code is ready for review again.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented May 6, 2016

@Simplify Does that mean that tslint doesn't handle syntax errors gracefully? Would you mind to show an example of what tslint outputs when there's a syntax error in the linted file?

I think graceful handling of syntax errors is really mandatory for us, because we have no other tslint syntax checker and Flycheck should really work out of the box, and not rely upon a 3rd party mode for something as basic as syntax checking.

@Simplify

This comment has been minimized.

Member

Simplify commented May 6, 2016

@lunaryorn TSLint will work even when there is error in ts file. I made https://github.com/Simplify/flycheck-typescript-tslint somewhere in November last year and never got any complains/issues from users. I use it personally on two projects.

// space between ex and port is a problem
ex port interface SomeInterface {
  name: string;
}

TSLint will complain about missing semicolons after ex (line 2, char 3) and port (line 2, char 8).

flycheck-typescript-tide checker from tide-mode will complain about:

  • Cannot find name 'ex' - line 2, char 1
  • ';' expected - line 2 char 4
  • Cannot find name 'port' - line 2, char 4
  • ';' expected - line 2 char 9

That example code will not compile.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented May 6, 2016

@Simplify Do you think that we should have a test specifically for this?

@Simplify

This comment has been minimized.

Member

Simplify commented May 6, 2016

@lunaryorn test for what exactly? Testing TSLint output with typescript source file that doesn't compile? I can do that, but don't see much benefit. TSLint doesn't care of file actually compiles or not.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented May 6, 2016

@Simplify I'm not sure whether I can follow you. How can tslint not care whether a file compiles? I presume it has to parse files somehow, so what happens when a file has a really broken syntax? Like not just a wrong keyword, but like a missing opening brace, missing semicolons, etc.?

How does it handle those situations where the syntax is too broken to even recognize identifiers, etc.?

Or does it not even parse typescript and thus ignores syntax errors? But if it doesn't parse typescript how can it even successfully lint it?

@Simplify

This comment has been minimized.

Member

Simplify commented May 6, 2016

I think that the best way to explain it is screenshot:

screen shot 2016-05-06 at 15 08 45

Rrrors are from tide-mode, that uses typescript compilers. Warnings are from tslint. File is completely wrong on so many levels and TSLint just does his part.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented May 6, 2016

I see, but the documentation states:

Please ensure that the TypeScript source files compile correctly before running the linter.

I'm not sure whether I'm in favour of relying on what appears a purely accidental feature of tslint.

@Simplify

This comment has been minimized.

Member

Simplify commented May 6, 2016

I see. So far I never had problems with TSLint output. I'll try tomorrow to "break" TSLint with input files somehow. Today I did check TSLint source code to see when TSLint throws errors, just to see how can be broken. throw is used only when using wrong arguments (not possible in our case because of the way how flycheck uses arguments) and when using formatters (we don't use those, just TSLint standard JSON output format). I'll investigate this in more details tomorrow.

@Simplify

This comment has been minimized.

Member

Simplify commented May 7, 2016

@lunaryorn I did several tests this morning. TSLint will just parse everything with .ts extension. I even renamed some ruby and html files to .ts and TSLint just parse those. I really don't expect any problems with this checker.

My biggest concern is smooth transaction for current flycheck-typescript-tslint users. I just released new version with same JSON parser as in this PR. It will be on Melpa in 3-4 hours. I'm using JSON parser in my projects few days without any problems.

When this PR gets merged into master, I'm planning to remove all code from my package and just add display-warning in after-init-hook with message that typescript-tslint is now part of flycheck and that users can uninstall package and remove all references from Emacs init file. I have also to change package description with some deprecation tekst, just to stop new installs. In a week or two I'l create PR in Melpa for removal of flycheck-typescript-tslint.

There is always a chance that somebody is using flycheck from melpa-stable and flycheck-typescript-tslint from standard "unstable" Melpa. We can just ignore those or you can release new version of flycheck (27?) with this PR in both Melpa's around same time if possible.

I'm interested in your opinion about all of this.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented May 7, 2016

@Simplify That sound promising.

I'd not bother too much about a transition period. In fact, I'd just remove the flycheck-typescript-tslint package from MELPA once this PR is merged. We can definitely make a new release of Flycheck to catch up with MELPA Stable users, so I think everyone should be fine.

@Simplify

This comment has been minimized.

Member

Simplify commented May 7, 2016

@lunaryorn Ok, that makes everything easier :)

@lunaryorn lunaryorn closed this in 9b8bf53 May 7, 2016

@lunaryorn lunaryorn removed the S-to review label May 7, 2016

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented May 7, 2016

@Simplify I've merged this PR, we now have typescript support in Flycheck 👍

Would you like to join Flycheck to maintain typescript support in Flycheck and help to make Flycheck better?

@Simplify

This comment has been minimized.

Member

Simplify commented May 7, 2016

@lunaryorn sure thing :)

@Simplify

This comment has been minimized.

Member

Simplify commented May 7, 2016

and thanks :)

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented May 7, 2016

@Simplify Great, I've send you an invitation to our organisation.

Please read through our Contributor's Guide and our Maintainer's Guide to see how we work, and please don't hesitate to ask any questions if anything is unclear.

Welcome on board! /cc @flycheck/maintainers

@Simplify

This comment has been minimized.

Member

Simplify commented May 7, 2016

@lunaryorn Thanks a lot :)
I'll also try to help with other staff that I use daily, ruby, (s)css, js, swift,...

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented May 7, 2016

@Simplify Great, awesome! It's cool to have you on board.

I'll add you to our to our people page on the website asap. Mind if I add you to a new JavaScript team as well?

@Simplify

This comment has been minimized.

Member

Simplify commented May 7, 2016

@lunaryorn yes, it's ok.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented May 7, 2016

@Simplify Great, thanks. I'll make a release tomorrow to push the new syntax checker to MELPA Stable.

@Simplify

This comment has been minimized.

Member

Simplify commented May 8, 2016

@lunaryorn Ok, flycheck-typescript-tslint is removed from Melpa.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented May 8, 2016

@Simplify And I've just pushed out Flycheck 27 with the new tslint syntax checker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment