Implemented fixer for member-ordering and added corresponding tests. #3935

NaridaL · 2018-06-01T20:39:21Z

PR checklist

Addresses an existing issue: fixes Feature Request: correct member-ordering issues with the --fix flag #2023
New feature, bugfix, or enhancement
Includes tests
Documentation update

Overview of change:

New fixer. See checkMembers JSDoc for implementation details.

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

test/rules/member-ordering/alphabetize/ is failing, but the diff isn't showing anything. The .fix doesn't exactly mirror the error message in some cases, as many configurations have multiple correct orders; that shouldn't be an issue.

I'm currently using node.getFullStart() and node getEnd() to find the node source, however this doesn't match the typescript trivia ownership rules (see failing test test/rules/member-ordering/fix-trivia). I've seen ts.getLeadingCommentRanges, but that starts at the beginning of the comment and whitespace before the leading comments would get lost. It seems like the best way would be to split at the first line break after a node, but I haven't seen any corresponding utility functions. Anyone have any advice on the matter or a reference to another rule which does something similar?

CHANGELOG.md entry: [new-fixer] `member-ordering`

palantirtech · 2018-06-01T20:39:24Z

Thanks for your interest in palantir/tslint, @NaridaL! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

NaridaL · 2018-06-01T20:40:51Z

NaridaL · 2018-06-10T09:22:55Z

Anyone available to review this PR?

listepo-alterpost · 2018-06-13T21:31:52Z

@ajafff ?

giladgray

a (mostly) green build is a good indication that your code is ready for review.

giladgray · 2018-06-14T17:38:49Z

src/rules/memberOrderingRule.ts

+            const sortedMembers = members.slice().sort((a, b) => {
+                // first, sort by member rank
+                // const ai = info(a);
+                // const bi = info(b);


no commented code

giladgray · 2018-06-14T17:39:17Z

src/rules/memberOrderingRule.ts

+/**
+ * Array.prototype.findIndex, but the last index.
+ */
+function arrayFindLastIndex<T>(


https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/lastIndexOf ?

Yeah, that doesn't support a custom predicate.

giladgray · 2018-06-14T17:39:54Z

src/rules/memberOrderingRule.ts

+}
+
+/**
+ * Applies a Replacement to a part of the text which starts at offset.


this comment does not match the impl. it returns a string??

It's just a slightly modified version of Replacement.apply https://github.com/palantir/tslint/blob/master/src/language/rule/rule.ts#L204, which also returns a string.

giladgray · 2018-06-14T17:40:49Z

src/rules/memberOrderingRule.ts

@@ -250,7 +270,8 @@ class MemberOrderingWalker extends Lint.AbstractWalker<Options> {
                    : "at the beginning of the class/interface";
                const errorLine1 = `Declaration of ${nodeType} not allowed after declaration of ${prevNodeType}. ` +
                    `Instead, this should come ${locationHint}.`;
-                this.addFailureAtNode(member, errorLine1);
+                this.addFailureAtNode(member, errorLine1, []);


why the addition of empty array? seems unnecessary

The empty array is needed so the Replacement can later be pushed (L241). The field itself is readonly. I copied this tidbit from the ordered-imports rule. https://github.com/palantir/tslint/blob/master/src/rules/orderedImportsRule.ts#L265

a code comment would clarify this for the future.

NaridaL · 2018-06-14T18:40:55Z

@giladgray Thanks for the review, do you have anything to add re: the two points I mentioned in the first post?

NaridaL · 2018-06-15T09:18:49Z

test/rules/member-ordering/alphabetize/ had a switched empty line and non-empty line, which wasn't showing up in the diff.

I went ahead and implemented a function to figure out the next newline after a node.

This should be mergeable now.

giladgray · 2018-06-18T16:44:12Z

@NaridaL merge latest master to fix failing test.

giladgray

almost there.

giladgray · 2018-06-18T16:45:03Z

src/rules/memberOrderingRule.ts

@@ -250,7 +270,8 @@ class MemberOrderingWalker extends Lint.AbstractWalker<Options> {
                    : "at the beginning of the class/interface";
                const errorLine1 = `Declaration of ${nodeType} not allowed after declaration of ${prevNodeType}. ` +
                    `Instead, this should come ${locationHint}.`;
-                this.addFailureAtNode(member, errorLine1);
+                this.addFailureAtNode(member, errorLine1, []);


a code comment would clarify this for the future.

giladgray · 2018-06-18T16:45:12Z

src/rules/memberOrderingRule.ts

@@ -272,6 +294,54 @@ class MemberOrderingWalker extends Lint.AbstractWalker<Options> {
                prevRank = rank;
            }
        }
+        if (failureExists) {
+            // const info = (x: Node)=>[x.name && nameString(x.name), this.memberRank(x), members.indexOf(x)]


remove commented code

giladgray · 2018-06-18T16:47:05Z

src/rules/memberOrderingRule.ts

+                    // if yes, remove it from the list so that we do not return overlapping Replacements
+                    const fixIndex = arrayFindLastIndex(
+                        this.fixes,
+                        ([, r]) => r.start >= start && r.start + r.length <= end,


🙊 didn't know you could destructure an array and skip indices!!

…correct nodes.

NaridaL · 2018-06-18T16:55:20Z

I rebased and fixed the comments.

giladgray

@NaridaL this is awesome stuff! great job on tackling such a tricky feature.

NaridaL · 2018-06-18T17:06:08Z

@giladgray Are you sure this fixes #3965 ?

giladgray · 2018-06-26T15:12:44Z

@NaridaL ~~you wrote this... are you sure that it fixes it?~~

edit: ah gotcha, changed language in description.

NaridaL · 2018-06-26T15:52:46Z

This doesn't actually do anything for #3965 . #3984 does, however.

…alantir#3935) * Implemented fixer for member-ordering and added corresponding tests. * Improved node boundary calculation so trivia remains attached to the correct nodes. * minor comment fixes.

Justinidlerz · 2018-07-13T07:18:15Z

Wow, I'm look forward to the member-ordering feature long time, Could publish an new npm beta version? I want to use it now.

roman-petrov · 2018-07-13T20:23:12Z

@Justinidlerz same for our team. Member ordering is hard to fix manually in a big project. @NaridaL, thank you for great feature! looking forward to see next release soon

giladgray suggested changes Jun 14, 2018

View reviewed changes

NaridaL mentioned this pull request Jun 16, 2018

member-ordering rule, alphabetize not working on getters and setters #3965

Closed

giladgray suggested changes Jun 18, 2018

View reviewed changes

NaridaL added 3 commits June 18, 2018 18:51

Implemented fixer for member-ordering and added corresponding tests.

ba7f1ff

Improved node boundary calculation so trivia remains attached to the …

7b229fc

…correct nodes.

minor comment fixes.

625b307

giladgray approved these changes Jun 18, 2018

View reviewed changes

NaridaL mentioned this pull request Jun 21, 2018

[bugfix] member-ordering includes getters and setters #3984

Merged

4 tasks

giladgray merged commit f82ef3c into palantir:master Jun 26, 2018

NaridaL deleted the member-ordering-fixer branch June 26, 2018 21:06

panchalkalpesh mentioned this pull request Nov 9, 2018

Pull v5.11.0 panchalkalpesh/tslint#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented fixer for member-ordering and added corresponding tests. #3935

Implemented fixer for member-ordering and added corresponding tests. #3935

NaridaL commented Jun 1, 2018 •

edited by giladgray

Loading

palantirtech commented Jun 1, 2018

NaridaL commented Jun 1, 2018

NaridaL commented Jun 10, 2018

listepo-alterpost commented Jun 13, 2018

giladgray left a comment

giladgray Jun 14, 2018

giladgray Jun 14, 2018

NaridaL Jun 14, 2018

giladgray Jun 14, 2018

NaridaL Jun 14, 2018

giladgray Jun 14, 2018

NaridaL Jun 14, 2018

giladgray Jun 18, 2018

NaridaL commented Jun 14, 2018

NaridaL commented Jun 15, 2018

giladgray commented Jun 18, 2018

giladgray left a comment

giladgray Jun 18, 2018

giladgray Jun 18, 2018

giladgray Jun 18, 2018

NaridaL commented Jun 18, 2018

giladgray left a comment

NaridaL commented Jun 18, 2018 •

edited

Loading

giladgray commented Jun 26, 2018 •

edited

Loading

NaridaL commented Jun 26, 2018

Justinidlerz commented Jul 13, 2018 •

edited

Loading

roman-petrov commented Jul 13, 2018

Implemented fixer for member-ordering and added corresponding tests. #3935

Implemented fixer for member-ordering and added corresponding tests. #3935

Conversation

NaridaL commented Jun 1, 2018 • edited by giladgray Loading

PR checklist

Overview of change:

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

CHANGELOG.md entry: [new-fixer] member-ordering

palantirtech commented Jun 1, 2018

NaridaL commented Jun 1, 2018

NaridaL commented Jun 10, 2018

listepo-alterpost commented Jun 13, 2018

giladgray 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NaridaL commented Jun 14, 2018

NaridaL commented Jun 15, 2018

giladgray commented Jun 18, 2018

giladgray 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

NaridaL commented Jun 18, 2018

giladgray left a comment

Choose a reason for hiding this comment

NaridaL commented Jun 18, 2018 • edited Loading

giladgray commented Jun 26, 2018 • edited Loading

NaridaL commented Jun 26, 2018

Justinidlerz commented Jul 13, 2018 • edited Loading

roman-petrov commented Jul 13, 2018

NaridaL commented Jun 1, 2018 •

edited by giladgray

Loading

CHANGELOG.md entry: [new-fixer] `member-ordering`

NaridaL commented Jun 18, 2018 •

edited

Loading

giladgray commented Jun 26, 2018 •

edited

Loading

Justinidlerz commented Jul 13, 2018 •

edited

Loading