Add exceptions support for comment-format rule (fixes #562) #1757

IllusionMH · 2016-11-20T02:17:29Z

PR checklist

Addresses an existing issue: comment-format rule "check-lowercase" option should allow whitelist of exceptions #562
~~New feature, bugfix, or~~ enhancement
- Includes tests
Documentation update

What changes did you make?

This PR adds support for exceptions list to comment-format rule.
Implementation uses array of exceptions as last argument.

Is there anything you'd like reviewers to focus on?

1. Is there a need to escape special characters in exception list to avoid broken RegEx'es?
2. Does exceptions array in last position good choice until #1738 implemented?

P.S. Is it possible to set tests timeout for executable to 3000ms? This will help me a lot, since some tests take ~2500ms under when I run tests in Docker container.

nchen63 · 2016-11-21T21:48:01Z

@IllusionMH I extended timeout for executable to 4s in #1766

nchen63 · 2016-11-21T22:00:01Z

src/rules/commentFormatRule.ts

+                    type: "string",
+                    enum: ["check-space", "check-lowercase", "check-uppercase"],
+                }, {
+                    type: "array",


would it be better to just make this a single regex string since the code makes it into a reg expression anyways? Also, people might see weird behavior if they don't know it's a regex.

on second thought, it's probably better the way it is...

I've checked list of ESLint rules and they use RE to provide patterns, however they use object with "ignorePattern" key to provide it.

As alternative approach I can implement exception argument as object with two possible keys ignoreWords - array of strings to ignore and ignorePattern - string with RE to ignore, instead of array of words. Users can select either of two, which suit best for their needs.

yes, would suggest an object with key for future extensibility

Ok. I'll rework this PR to use object instead of array.
And one more question: Is current state of options in rule metadata correct for case with array?

I not sure if I correctly get this format especially when there should be 2 types of items in array - strings and array (unfortunately I can't find docs with description or sources that process it)

I'm not that familiar with JSON schema, but don't you want anyOf instead of oneOf?

Thanks for the tip, I'll check Schema docs then. (haven't used JSON Schema before and didn't realize that TSLint uses it)

IllusionMH · 2016-11-21T22:53:11Z

@IllusionMH I extended timeout for executable to 4s in #1766

@nchen63 thank you!

I've noticed that I forgot to update optionsDescription, and I will submit fix later this week (and address any other requests).

adidahiya · 2016-12-12T17:56:07Z

@IllusionMH could you please enable Circle CI on your fork?

IllusionMH · 2016-12-12T21:04:54Z

@adidahiya added. But for some reason it doesn't run build on all 3 Node.js versions.
Also I haven't synced this branch with master.

adidahiya · 2016-12-12T22:04:30Z

Your CI build looks fine, it is running different node versions in 3 containers (maybe you went and enabled multiple containers in the hour since you posted your last comment :))

IllusionMH · 2016-12-12T22:14:33Z

@adidahiya yes, now it runs in 3 containers, however failed/stopped few times when I tried to setup them.

Moreover build on master runs fine in my fork (both have same last commit d053355 ) 😃

Do I need to do any other actions with CI?

palantir#562)

IllusionMH · 2017-01-07T00:19:50Z

I've rebased this branch and resolved conflicts.

@nchen63, @adidahiya is there anything that should be addressed in this PR?

nchen63

Sorry the review took so long

nchen63 · 2017-01-07T05:43:56Z

src/rules/commentFormatRule.ts

        type: "style",
        typescriptOnly: false,
    };
    /* tslint:enable:object-literal-sort-keys */

-    public static LOWERCASE_FAILURE = "comment must start with lowercase letter";
-    public static UPPERCASE_FAILURE = "comment must start with uppercase letter";
+    public static LOWERCASE_FAILURE = "comment must start with lowercase letter, word from exceptions list or exceptions pattern";


Could you print the list of exceptions or regex exception in the error? I think most people wouldn't know what an "exceptions list" is when seeing this error.

Thanks! Good point.
Is it ok if there will be 3 different messages instead of one generic?
comment must start with lowercase letter
comment must start with lowercase letter or word from exceptions list: TODO, HACK
comment must start with lowercase letter or match exceptions pattern: STD(IN|OUT|ERR)\b

And same for uppercase option.

3 different messages are ok.

I'd avoid using the word "exceptions" since it could confuse people with the kind of exception you throw. Something like Comment must start with a lowercase letter, or the words "TODO", or "HACK" and Comment must start with a lowercase letter or match the regex pattern "STD(IN|OUT|ERR)\b

IllusionMH · 2017-01-08T03:30:49Z

@nchen63 I've updated messages.
Message for ignore words was simplified to avoid differences for cases where single or several words are specified.
Message for ignore pattern was extended to specify that start of the comment will be matched against regexp and not a whole comment.

nchen63 · 2017-01-08T09:15:59Z

@IllusionMH thanks!

nchen63 · 2017-01-23T19:36:17Z

I realized as I was compiling the release notes that the option ignoreWords should probably be ignore-words

IllusionMH · 2017-01-23T20:23:29Z

@nchen63 you are right. typedef-whitespace has config object with hyphens in keys.
I have stupid question: when rule docs should be updated? In every pull request or they are generated right before release?
Docs don't describe requirement here and I remember cases when docs weren't required between 3.15 and 4.0 releases.

nchen63 · 2017-01-23T21:35:33Z

Docs are generated and published with the release. I've recently added the generated docs to .gitignore, so it's not possible to update in any case.

IllusionMH · 2017-01-23T21:36:42Z

@nchen63 thanks, PR is on the way

nchen63 reviewed Nov 21, 2016

View reviewed changes

adidahiya assigned adidahiya and unassigned adidahiya Dec 13, 2016

IllusionMH added 3 commits January 7, 2017 02:06

Add exceptions support for comment-format rule (fixes palantir#562)

f8c43dc

Add support for RexExp pattern in comment-format rule exceptions (fixes

c8a03f4

palantir#562)

Fix escapeRegExp and add test

6fe20ce

IllusionMH force-pushed the comment-format-exceptions branch from aba85ee to 6fe20ce Compare January 7, 2017 00:14

nchen63 suggested changes Jan 7, 2017

View reviewed changes

Make failure messages more detailed

26167bb

nchen63 approved these changes Jan 8, 2017

View reviewed changes

nchen63 merged commit 11d1281 into palantir:master Jan 8, 2017

IllusionMH deleted the comment-format-exceptions branch January 8, 2017 12:23

nchen63 mentioned this pull request Jan 23, 2017

Prepare for v4.4.0 #2109

Merged

3 tasks

IllusionMH mentioned this pull request Jan 23, 2017

Make keys consistent with other options #2110

Merged

4 tasks

keylocation-bot mentioned this pull request Jan 25, 2017

Update dependency tslint to version 4.4.2 singapore/lint-condo#223

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add exceptions support for comment-format rule (fixes #562) #1757

Add exceptions support for comment-format rule (fixes #562) #1757

IllusionMH commented Nov 20, 2016 •

edited

Loading

nchen63 commented Nov 21, 2016

nchen63 Nov 21, 2016

nchen63 Nov 21, 2016

IllusionMH Nov 21, 2016 •

edited

Loading

nchen63 Nov 22, 2016

IllusionMH Nov 22, 2016

nchen63 Nov 24, 2016

IllusionMH Nov 24, 2016

IllusionMH commented Nov 21, 2016

adidahiya commented Dec 12, 2016

IllusionMH commented Dec 12, 2016

adidahiya commented Dec 12, 2016

IllusionMH commented Dec 12, 2016 •

edited

Loading

IllusionMH commented Jan 7, 2017

nchen63 left a comment

nchen63 Jan 7, 2017

IllusionMH Jan 7, 2017 •

edited

Loading

nchen63 Jan 7, 2017 •

edited

Loading

IllusionMH commented Jan 8, 2017

nchen63 commented Jan 8, 2017

nchen63 commented Jan 23, 2017

IllusionMH commented Jan 23, 2017

nchen63 commented Jan 23, 2017

IllusionMH commented Jan 23, 2017

Add exceptions support for comment-format rule (fixes #562) #1757

Add exceptions support for comment-format rule (fixes #562) #1757

Conversation

IllusionMH commented Nov 20, 2016 • edited Loading

PR checklist

What changes did you make?

Is there anything you'd like reviewers to focus on?

nchen63 commented Nov 21, 2016

nchen63 Nov 21, 2016

Choose a reason for hiding this comment

nchen63 Nov 21, 2016

Choose a reason for hiding this comment

IllusionMH Nov 21, 2016 • edited Loading

Choose a reason for hiding this comment

nchen63 Nov 22, 2016

Choose a reason for hiding this comment

IllusionMH Nov 22, 2016

Choose a reason for hiding this comment

nchen63 Nov 24, 2016

Choose a reason for hiding this comment

IllusionMH Nov 24, 2016

Choose a reason for hiding this comment

IllusionMH commented Nov 21, 2016

adidahiya commented Dec 12, 2016

IllusionMH commented Dec 12, 2016

adidahiya commented Dec 12, 2016

IllusionMH commented Dec 12, 2016 • edited Loading

IllusionMH commented Jan 7, 2017

nchen63 left a comment

Choose a reason for hiding this comment

nchen63 Jan 7, 2017

Choose a reason for hiding this comment

IllusionMH Jan 7, 2017 • edited Loading

Choose a reason for hiding this comment

nchen63 Jan 7, 2017 • edited Loading

Choose a reason for hiding this comment

IllusionMH commented Jan 8, 2017

nchen63 commented Jan 8, 2017

nchen63 commented Jan 23, 2017

IllusionMH commented Jan 23, 2017

nchen63 commented Jan 23, 2017

IllusionMH commented Jan 23, 2017

IllusionMH commented Nov 20, 2016 •

edited

Loading

IllusionMH Nov 21, 2016 •

edited

Loading

IllusionMH commented Dec 12, 2016 •

edited

Loading

IllusionMH Jan 7, 2017 •

edited

Loading

nchen63 Jan 7, 2017 •

edited

Loading