Track and restore initial or previous rule state for one line lint switches (fixes #1624) #1634

IllusionMH · 2016-10-16T04:25:17Z

This PR makes EnableDisableRulesWalker aware of initial rules configuration for file.
This allows track rule state during file parsing and restore proper rule state after it was disabled for single line (with disable-line or disable-next-line switchers).

Todo:

Handle tslint:(disable|enable) cases
~~Add tests for enable-line and enable-next-line switchers~~ (it was early morning and I've missed that this rules aren't documented)
Do not add new entries to enableDisableRuleMap if rule state doesn't change

ETA: October, 23rd

IllusionMH · 2016-10-23T23:47:44Z

I've updated this pull request with rework for how enabled/disabled states are tracked for file.
Notable changes:

Rule switchers are tracked only for rules that are enabled in configuration (rules that wouldn't be executed in any case)
Switch positions are only added in case when state for rule changes (this allow to be sure that states in enableDisableRuleMap always alternate which simplifies generation of disabled intervals)
If switch is performed for all rules - all of them are manually switched if needed (further simplifies generation of disabled intervals)

I've removed test cases for enable-line and enable-next-line from TODO list since this feature isn't documented for TSLint (ESLint also).
@adidahiya @jkillian should this feature be supported (I'll add test cases if needed) or may be it should show warnings for developers (since it might be error prone to count on rule that are enabled only for one line in disabled interval) ?

At this point I think that work is finished and PR is ready for review.

IllusionMH · 2016-11-12T01:43:36Z

I haven't found any docs on my question so I want to clarify it here: What is proper way for this repo to make branch up to date with master - git rebase master or git merge master?

nchen63 · 2016-11-12T02:20:31Z

It doesn't matter if you merge or rebase since we just squash the commit before merging into master

IllusionMH · 2016-11-12T03:50:54Z

src/tslintMulti.ts

@@ -164,19 +164,19 @@ class MultiLinter {

    private getEnabledRules(fileName: string, source?: string, configuration: IConfigurationFile = DEFAULT_CONFIG): IRule[] {
        const sourceFile = this.getSourceFile(fileName, source);
+        const isJs = fileName.substr(-3) === ".js";


Found this during PR update.
I believe that this check should include .jsx files too. Something like /\.jsx?$/i.test(fileName)

yes, good catch

IllusionMH · 2016-11-12T03:55:45Z

Rebased and updated PR with recent changes in master.

…antir#1624)

nchen63

looks good in general. The enable-line and enable-next-line cases still work right? Like nothing blows up?

nchen63 · 2016-11-24T04:50:45Z

src/enableDisableRules.ts

+        });
+
+        if (end) {
+            // switchRule method is only called when rule state changes therefore we can safely use opposite state


switchRule -> switchRuleState

nchen63 · 2016-11-24T05:00:37Z

src/ruleLoader.ts

- * We're assuming both lists are already sorted top-down so compare the tops, use the smallest of the two,
- * and build the intervals that way.
+/**
+ * creates disabled intervals for rule based on list of swithers for it


swithers -> switchers

nchen63 · 2016-11-25T06:03:00Z

src/enableDisableRules.ts

+        }
+
+        // use same logic as in AbstractRule
+        function isEnabled(value: any): boolean {


factor out and put in Utils

IllusionMH · 2016-11-27T01:13:54Z

@nchen63 thank you for review.

The enable-line and enable-next-line cases still work right? Like nothing blows up?

Yes. Actually there already were tests for enable-line/enable-next-line switchers.

factor out and put in Utils

src/utils.ts contains only basic utility functions without any rules related functionality.
Therefore I decided to move logic from instance isEnabled method to static method which still belongs to AbstractRule.
But if you insist - I'll move it to utils.

nchen63 · 2016-11-27T04:52:17Z

@IllusionMH Looks good. Thanks!

adidahiya added Status: Work In Progress PR: waiting for reviewer labels Oct 16, 2016

IllusionMH changed the title ~~[WIP] Track and restore initial or previous rule state for one line lint switches (fixes #1624)~~ Track and restore initial or previous rule state for one line lint switches (fixes #1624) Oct 23, 2016

adidahiya removed the Status: Work In Progress label Oct 27, 2016

IllusionMH force-pushed the enable-disable-line-fix branch from 99a8f7f to 0e26676 Compare November 12, 2016 03:48

IllusionMH commented Nov 12, 2016

View reviewed changes

IllusionMH mentioned this pull request Nov 13, 2016

Use jsRules for JSX files #1714

Merged

4 tasks

IllusionMH added 7 commits November 16, 2016 01:31

Track and restore initial or previous rule state (fixes palantir#1624)

7fdd1f8

Remove unused parameter. Fix code compilation.

8290225

Add disable-next-line to test case

01638ad

Track rule state changes for all enabled rules it the file (fixes pal…

5437e71

…antir#1624)

Minor style fix. Added missing spaces

172505b

Properly use JS configuration rules if needed

8b4cf8b

Update PR with new API

471d162

IllusionMH force-pushed the enable-disable-line-fix branch from 0e26676 to 471d162 Compare November 16, 2016 00:07

nchen63 reviewed Nov 26, 2016

View reviewed changes

nchen63 removed the PR: waiting for reviewer label Nov 26, 2016

IllusionMH added 3 commits November 27, 2016 02:03

Merge branch 'master' into enable-disable-line-fix

8671f78

Fix typos in comments

e84ad30

Mode default isEnabled check for rules to static method

60015fd

nchen63 merged commit f1df7b7 into palantir:master Nov 27, 2016

IllusionMH deleted the enable-disable-line-fix branch November 27, 2016 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track and restore initial or previous rule state for one line lint switches (fixes #1624) #1634

Track and restore initial or previous rule state for one line lint switches (fixes #1624) #1634

IllusionMH commented Oct 16, 2016 •

edited

Loading

IllusionMH commented Oct 23, 2016

IllusionMH commented Nov 12, 2016

nchen63 commented Nov 12, 2016

IllusionMH Nov 12, 2016

nchen63 Nov 12, 2016

IllusionMH commented Nov 12, 2016

nchen63 left a comment

nchen63 Nov 24, 2016

nchen63 Nov 24, 2016

nchen63 Nov 25, 2016

IllusionMH commented Nov 27, 2016

nchen63 commented Nov 27, 2016

Track and restore initial or previous rule state for one line lint switches (fixes #1624) #1634

Track and restore initial or previous rule state for one line lint switches (fixes #1624) #1634

Conversation

IllusionMH commented Oct 16, 2016 • edited Loading

IllusionMH commented Oct 23, 2016

IllusionMH commented Nov 12, 2016

nchen63 commented Nov 12, 2016

IllusionMH Nov 12, 2016

Choose a reason for hiding this comment

nchen63 Nov 12, 2016

Choose a reason for hiding this comment

IllusionMH commented Nov 12, 2016

nchen63 left a comment

Choose a reason for hiding this comment

nchen63 Nov 24, 2016

Choose a reason for hiding this comment

nchen63 Nov 24, 2016

Choose a reason for hiding this comment

nchen63 Nov 25, 2016

Choose a reason for hiding this comment

IllusionMH commented Nov 27, 2016

nchen63 commented Nov 27, 2016

IllusionMH commented Oct 16, 2016 •

edited

Loading