Add text replacement fixes for rules #1423

ScottSWu · 2016-07-21T20:25:31Z

Created IFix and IReplacement structures for representing fixes
- programWalker includes createFix and createReplacement
Added optional parameter to createFailure for suggesting fixes
Modified semicolon rule to suggest fixes (add / remove semicolon)
Modified testing to check against a file with fixes applied
- If there is a ".ts.fix" file, then the test will take the first suggested fix and apply to the test file (without markup) and compare

Addresses #561

jkillian · 2016-07-22T02:14:37Z

@alexeagle, do you plan to review this? I doubt I'll be able to get to this until sometime next week at the earliest. Definitely a great feature I want to get implemented!

ScottSWu · 2016-07-22T16:07:35Z

cc: @mprobst

alexeagle · 2016-07-22T16:34:52Z

I've reviewed it on Scott's branch. I'll take another pass here, but the design already LGTM

alexeagle · 2016-07-22T16:37:26Z

src/test.ts

+            if (stat.isFile()) {
+                fixedFileText = fs.readFileSync(fixedFile, "utf8");
+                // accumulate replacements
+                const replacements = failures.reduce((acc, failure) => {


I'm not sure the reduce here is worth the syntactic and cognitive cost, it might be simpler to just iterate?

jkillian · 2016-07-29T02:46:09Z

src/language/rule/rule.ts

+    constructor(private start: number, private length: number, private text: string) {
+    }
+
+    public getStart() {


I'd be fine with making all of these real get methods. I find that this sort of method can get frustrating (but fine either way)

jkillian · 2016-07-29T02:52:45Z

Looking good @ScottSWu.

Is the Replacement API robust enough? How tricky to use is it when you get into weird changes, like overlapping fixes? What about when multiple rules want to apply fixes to the same file? I know you looked into the way it's done in other similar tools.

Also, a test or two for your new test code would be great if possible! You could just add it to test/check-bin.sh

alexeagle · 2016-07-29T17:09:48Z

Applying fixes from multiple rules at once sounds like trouble to me. In the worst case, two rules may conflict, and after applying one set of replacements, you are left with some that don't apply, and now the result isn't even syntactically valid.
I think we'd have to apply the fixes (either to the filesystem or an in-memory buffer) after running one rule, then take the result and pass it to the next rule. You may get different results depending on the order the rules are executed.

Within a single rule, I'm not sure there is a need for overlapping fixes. Fixes should prepend or append text where possible, rather than replacing a whole range (and it's hard to convert an AST subtree to text anyway)

ScottSWu · 2016-07-29T21:20:02Z

To add on a bit, no these replacement functions are not robust, but would be building blocks for an something that is. A single fix should not have overlapping replacements within itself, but also should be applied entirely or not at all (e.g. it doesn't make sense to replace one quote mark). Thus the Fix.apply and the Replacement.apply functions will be useful.

From what ESLint does, fixes are automatically applied, and only fixes that failed to be applied are displayed.

EDIT:
This current PR doesn't include the CLI options, since we first wanted to figure out how to decide which rules to apply, and how to apply them. Are there other tests that would go in check-bin.sh?

* Created IFix and IReplacement structures for representing fixes * programWalker includes createFix and createReplacement * Added optional parameter to createFailure for suggesting fixes * Modified semicolon rule to suggest fixes * Modified testing to check against a file with fixes applied

- Replaced reduce with for loop - Changed Fix and Replacement to classes - Added apply method to each - Fix.apply applies all replacements in reverse order - Replacement.apply performs the text substitution - Added function to display diff results in the tester

alexeagle · 2016-08-03T16:30:22Z

Ping @jkillian - Scott's internship has only a couple more weeks so we'd like to get this moving

jkillian · 2016-08-03T19:47:32Z

src/language/walker/ruleWalker.ts

@@ -82,6 +82,14 @@ export class RuleWalker extends SyntaxWalker {
        }
    }

+    public createFix(replacements: Replacement[]): Fix {


I went back and forth on if we should have these wrapper methods at all (as the constructors have the exact same functionality). Do you think it makes sense to have them?

I'd prefer to expose only the typical fix methods. These are what we need for the current check and expose less API details to all the rules that might start using it.

so far we need
appendText(n: node, t: text) for putting a semicolon after the statement, and
delete(start: number, length: number) for deleting a semicolon?

Fix is probably redundant. Eventually we should proxy replacements (including createReplacement) to make sure replacements at the same spots are applied in the right order. It turns out sorting is not guaranteed to be stable, but adding an additional ordering counter breaks ties correctly.

jkillian · 2016-08-03T20:56:51Z

src/language/rule/rule.ts

+
+    public apply(content: string) {
+        // sort replacements in reverse so that diffs are properly applied
+        this.innerReplacements.sort((a, b) => b.end - a.end);


I'm thinking we could have a Replacements.applyAll that is parallel to Fix.applyAll?

jkillian · 2016-08-03T21:02:00Z

@ScottSWu As far as check-bin tests, I was thinking perhaps something like here just to make sure that the code paths dealing with .fix have a test that specifically hits them.

jkillian · 2016-08-03T21:04:10Z

This is looking good though, I think we can merge this shortly. @ScottSWu, @alexeagle what do you plan to proceed with after this merges? Implementing fixes for more rules?

ScottSWu · 2016-08-03T23:08:06Z

Got it, added a passing and failing check-bin tests.

I believe the next step is to add the --fix option to the CLI. Implementing more fixes for rules is definitely important. There's also adding more helper functions for creating replacements, and outputting fixes in the other formatters.

jkillian · 2016-08-04T06:37:08Z

Sweet, future plans sound good to me. Merged this so things can more easily continue!

ScottSWu · 2016-08-04T16:51:37Z

Awesome! Thanks for pushing this through!

* Created IFix and IReplacement structures for representing fixes * programWalker includes createFix and createReplacement * Added optional parameter to createFailure for suggesting fixes * Modified semicolon rule to suggest fixes (add / remove semicolon) * Modified testing to check against a file with fixes applied * If there is a ".ts.fix" file, then the test will take the first suggested fix and apply to the test file (without markup) and compare Addresses palantir#561

jkillian added the PR: waiting for reviewer label Jul 22, 2016

alexeagle reviewed Jul 22, 2016
View reviewed changes

ScottSWu force-pushed the fixes_main branch from 764e205 to 5d3f622 Compare July 27, 2016 00:01

adidahiya assigned jkillian Jul 27, 2016

jkillian reviewed Jul 29, 2016
View reviewed changes

jkillian added PR: waiting for author and removed PR: waiting for reviewer labels Jul 29, 2016

ScottSWu added 5 commits August 1, 2016 09:44

Review changes

bb336e7

- Replaced reduce with for loop - Changed Fix and Replacement to classes - Added apply method to each - Fix.apply applies all replacements in reverse order - Replacement.apply performs the text substitution - Added function to display diff results in the tester

Changed methods to getters, removed description from fixes

a8df88b

Rules produce at most one fix

64d8fd8

Add Fix.applyAll

46f4e38

jkillian reviewed Aug 3, 2016
View reviewed changes

Removed createFix, added helper Replacement methods.

00f94e0

ScottSWu force-pushed the fixes_main branch from 752c5ae to 00f94e0 Compare August 3, 2016 20:39

jkillian reviewed Aug 3, 2016
View reviewed changes

Add Replacement.applyAll and check-bin tests for fixes

07b8719

ScottSWu force-pushed the fixes_main branch from d96cbcc to 07b8719 Compare August 3, 2016 22:53

jkillian merged commit fc9a72d into palantir:master Aug 4, 2016

adidahiya removed the PR: waiting for author label Aug 16, 2016

jkillian mentioned this pull request Aug 19, 2016

Rules that can fix the problems they find #561

Closed

adidahiya mentioned this pull request Sep 1, 2016

CLI interface for rule fixes #1534

Closed

adidahiya mentioned this pull request Nov 3, 2016

Expose CLI functionality as a separate class #1675

Closed

nchen63 added this to the TSLint v4.0 milestone Nov 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add text replacement fixes for rules #1423

Add text replacement fixes for rules #1423

ScottSWu commented Jul 21, 2016 •

edited

Loading

jkillian commented Jul 22, 2016

ScottSWu commented Jul 22, 2016

alexeagle commented Jul 22, 2016

alexeagle Jul 22, 2016

ScottSWu Jul 25, 2016

jkillian Jul 29, 2016

ScottSWu Jul 29, 2016

jkillian commented Jul 29, 2016

alexeagle commented Jul 29, 2016

ScottSWu commented Jul 29, 2016 •

edited

Loading

alexeagle commented Aug 3, 2016

jkillian Aug 3, 2016

alexeagle Aug 3, 2016

ScottSWu Aug 3, 2016

jkillian Aug 3, 2016

jkillian commented Aug 3, 2016

jkillian commented Aug 3, 2016

ScottSWu commented Aug 3, 2016

jkillian commented Aug 4, 2016

ScottSWu commented Aug 4, 2016

Add text replacement fixes for rules #1423

Add text replacement fixes for rules #1423

Conversation

ScottSWu commented Jul 21, 2016 • edited Loading

jkillian commented Jul 22, 2016

ScottSWu commented Jul 22, 2016

alexeagle commented Jul 22, 2016

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

jkillian commented Jul 29, 2016

alexeagle commented Jul 29, 2016

ScottSWu commented Jul 29, 2016 • edited Loading

alexeagle commented Aug 3, 2016

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

jkillian commented Aug 3, 2016

jkillian commented Aug 3, 2016

ScottSWu commented Aug 3, 2016

jkillian commented Aug 4, 2016

ScottSWu commented Aug 4, 2016

ScottSWu commented Jul 21, 2016 •

edited

Loading

ScottSWu commented Jul 29, 2016 •

edited

Loading