Add TSLint to the project #430

nicknisi · 2017-02-21T04:33:56Z

Add a tslint.json file to the project, and clean up the files in src/ to follow it.

This PR touches a lot of files in order to standardize things such as === usage single quotes. Additionally, I added the noUnusedVariables to tsconfig.json and removed all unused imports and interfaces across the project.

This is part of the refactor changes detailed in #378.

add tslint to project and to build task, and setup a default tslint.json based on the current styles used in the project

jiayihu · 2017-02-21T11:52:30Z

test/converter/access/specs.json

@@ -20,6 +20,7 @@
          "kind": 128,
          "kindString": "Class",
          "flags": {
+            "0": true,


Why there are added zeros?

I'll look into this and why the CI build failed.

jiayihu · 2017-02-21T11:57:12Z

Wow great work! There are strange zeros appearing in specs.json, which may be the cause of travis failure.

Anyway I think we should discuss more about tslint rules. For example I personally prefer:

Trailing commas in objects, arrays and function calls
Keeping the type property: string = 'defaultValue' instead of property = 'defaultValue' because it assures I won't be tempted to change it to property = 12 in future.

jiayihu · 2017-02-21T11:58:22Z

src/lib/converter/converter.ts

@@ -31,102 +30,49 @@ export interface IConverterResult


 /**
- * Event callback definition for generic converter events.


Was this part removed because of unused vars?

Yeah, these were removed because tsc now complains that they're not used anywhere.

nicknisi · 2017-02-21T15:44:23Z

@jiayihu I tried to match the styles already present in the project when creating the config file to make it as easy as possible, with a few exceptions where I added in best practices with === and trailing commas, although I could go either way on the latter. I also had to pick one or the other with regard to quote mark, so I went with single quotes since I'm lazy and it's one less key to press.

In this PR I wanted to match as close as possible with what's there, but in addition to the two changes you mentioned, I would also like to possibly see the following:

"curly": true - enforce curly braces for if/else/etc.
"interface-name": [ true, "never-prefix" ] - Remove the need for I prefixes on interfaces, as that is no longer the recommended style in TypeScript.
"one-line": - Ensure that {, } catch, and } finally` are on the same line
"typedef-whitespace": [ true, ... ]: I think it'd be nice to have whitespace between variable names and type definitions (for example: const foo: string;)

These changes, if agreed upon, could come in as a separate PR to further improve the code style.

aciccarello · 2017-02-21T16:53:14Z

Great work! Haven't had a chance to review everything but I'm good with the tslint config. As for the config mentioned in the comments, I'd support what @nicknisi suggested. I'm also good with trailing commas in objects & arrays but I'd prefer to allow implicit let property = 'defaultValue' type definitions since it keeps things clean. Thanks @jiayihu for looking at everything.

nicknisi · 2017-02-21T18:52:49Z

I found the issue that was causing the 0s to show up in the specs and fixed it, and now the CI builds are passing!

aciccarello

LGTM! I tried to glance at all the triple equal changes since that actually has a semantic difference. Seemed like most of them were comparing like numbers, enums, or strings so hopefully there aren't any issues. Only other thing I noticed was some template strings that seemed unnecessary but that's largely a style thing.

Personally I prefer white space between the variable name and the type definition but if most of the files had no space I'm good with keeping it consistent.

aciccarello · 2017-02-22T06:24:21Z

src/lib/converter/plugins/CommentPlugin.ts

@@ -144,7 +144,7 @@ export class CommentPlugin extends ConverterComponent
        const comment = reflection.parent.comment;
        if (comment) {
            let tag = comment.getTag('typeparam', reflection.name);
-            if (!tag) tag = comment.getTag('param', '<' + reflection.name + '>');
+            if (!tag) tag = comment.getTag('param', `<${reflection.name}>`);


Not sure the template literal is any better here

For consistency I personally use template literal anywhere there is interpolation

aciccarello · 2017-02-22T06:25:40Z

src/lib/converter/plugins/GitHubPlugin.ts

    // `stdout` was added in:
    // https://github.com/shelljs/shelljs/commit/8a7f7ceec4d3a77a9309d935755675ac368b1eda#diff-c3bfabb5e6987aa21bc75ffd95a162d6
    // As of 2016-10-16, DefinitelyTyped's defs are for shelljs v0.3.0, but we're on 0.7.0
+    // tslint:disable-next-line:interface-name


Any idea why this interface was unprefixed?

I'm not sure why this was unprefixed. ¯_(ツ)_/¯

Possibly my fault when I started updating TypeDoc and needed to improve that definition. I'd just check the blame, but it can be made consistent. I'd love to have another pass through removing the prefixes and correcting whitespace and whatnot.

nicknisi · 2017-02-22T15:52:04Z

@aciccarello I agree on the spacing between types, and there is a TSLint rule for it. I'll submit a separate PR.

* curly: true - enforce curly braces on if/while/etc. blocks * no-consecutive-blank-lines: true * one-line - make sure { is on the same line * make sure there is a space before type definitions

nicknisi · 2017-02-23T16:27:50Z

Per @blakeembrey's comment, I went ahead with another passthrough to fix the whitespace rules and to remove the prefix I on interface names. Along with that, I renamed IOptionDeclaration to DeclarationOption so that the name doesn't conflict with the class OptionDeclaration in the same module, as they appear to be two different things.

jiayihu · 2017-02-23T17:04:39Z

to remove the prefix I on interface names

Are we all agree to this? I personally prefer I prefix to avoid confusion with classes.

nicknisi · 2017-02-23T17:14:00Z

I personally don't like the I prefix, and not having them matches up with the TypeScript coding guidelines: https://github.com/Microsoft/TypeScript/wiki/Coding-guidelines#names

aciccarello · 2017-02-24T04:46:06Z

I can go either way on interface prefixes. Following TS guidelines seems good to me. I haven't done anything with plugins.

Does anyone know if they use the interface names? If so I'd assume it would be a breaking change to rename the interfaces?

nicknisi · 2017-02-24T05:16:41Z

I think that because no interfaces are exported from src/index.ts that they aren't really part of the public API, but I could be wrong.

blakeembrey · 2017-02-24T06:02:04Z

@aciccarello I'll be sure to release this as a minor just in case 👍

mootari · 2017-12-23T16:30:04Z

All converter event descriptions still state

The listener should implement [[IConverterCallback]].
The listener should implement [[IConverterNodeCallback]].

I assume those references were left in by accident?

nicknisi added 4 commits February 20, 2017 22:24

add tslint.json to project

b1709fd

add tslint to project and to build task, and setup a default tslint.json based on the current styles used in the project

fix tslint errors in src directory

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

a7bcf2a

add noUnusedLocals to tsconfig.json, remove unused locals

53daa9a

add generated file changes

a7c5cc9

jiayihu reviewed Feb 21, 2017

View reviewed changes

fix spec build issue, revert spec changes

88263da

aciccarello approved these changes Feb 22, 2017

View reviewed changes

nicknisi force-pushed the tslint branch from 6661770 to 9171710 Compare February 23, 2017 02:31

update whitespace rules in tslint.json and apply

34a88f9

* curly: true - enforce curly braces on if/while/etc. blocks * no-consecutive-blank-lines: true * one-line - make sure { is on the same line * make sure there is a space before type definitions

nicknisi force-pushed the tslint branch from 9171710 to 34a88f9 Compare February 23, 2017 02:34

nicknisi added 2 commits February 23, 2017 10:23

remove prefix 'I' from interfaces

36654f0

remove unnecessary interface-name tslint overrides

e67a7bb

blakeembrey merged commit 181a86d into TypeStrong:master Feb 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TSLint to the project #430

Add TSLint to the project #430

nicknisi commented Feb 21, 2017

jiayihu Feb 21, 2017

nicknisi Feb 21, 2017

jiayihu commented Feb 21, 2017

jiayihu Feb 21, 2017

nicknisi Feb 21, 2017

nicknisi commented Feb 21, 2017

aciccarello commented Feb 21, 2017

nicknisi commented Feb 21, 2017

aciccarello left a comment

aciccarello Feb 22, 2017

jiayihu Feb 22, 2017

aciccarello Feb 22, 2017

nicknisi Feb 22, 2017

blakeembrey Feb 22, 2017

nicknisi commented Feb 22, 2017

nicknisi commented Feb 23, 2017

jiayihu commented Feb 23, 2017

nicknisi commented Feb 23, 2017

aciccarello commented Feb 24, 2017

nicknisi commented Feb 24, 2017

blakeembrey commented Feb 24, 2017

mootari commented Dec 23, 2017

		@@ -31,102 +30,49 @@ export interface IConverterResult


		/**
		* Event callback definition for generic converter events.

Add TSLint to the project #430

Add TSLint to the project #430

Conversation

nicknisi commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiayihu commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicknisi commented Feb 21, 2017

aciccarello commented Feb 21, 2017

nicknisi commented Feb 21, 2017

aciccarello left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicknisi commented Feb 22, 2017

nicknisi commented Feb 23, 2017

jiayihu commented Feb 23, 2017

nicknisi commented Feb 23, 2017

aciccarello commented Feb 24, 2017

nicknisi commented Feb 24, 2017

blakeembrey commented Feb 24, 2017

mootari commented Dec 23, 2017