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

NaridaL · 2018-06-21T14:01:04Z

PR checklist

Addresses an existing issue: fixes member-ordering rule, alphabetize not working on getters and setters #3965
New feature, bugfix, or enhancement
Includes tests
Documentation update

Overview of change:

Added categories "-accessor" for getters and setters. Added those categories to the presets. Based on #3935, merge that first.

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

CHANGELOG.md entry:

[bugfix] member-ordering includes property accessors (getters and setters).

giladgray · 2018-06-26T16:19:24Z

@NaridaL I just merged #3935 so go ahead and update this 👍

NaridaL · 2018-06-26T16:57:20Z

Done. Let's see if the lint still fails...

NaridaL · 2018-06-26T17:17:31Z

OK, yeah, for some reason running npm i locally resulted in me getting the error too.

ERROR: 193:5  member-ordering  Declaration of public instance method not allowed after declaration of private instance method. Instead, this should come after private instance fields.

The error doesn't make much sense, because tslint's tslint.json is using the old-style option "variables-before-functions", and accessors are neither. This is going to be a breaking change, as I added the accessors to the presets, and if you didn't happen to put them in the same location you're going to get errors now. Therefore I suggest doing what the comment on line 534 suggests and remove the old-style options entirely and update tslint.json.

/**
 * Convert from undocumented old-style options.
 * This is designed to mimic the old behavior and should be removed eventually.
 */

NaridaL · 2018-06-26T17:29:41Z

OK, I changed tslint's own config to use the current config type.

Can I remove the old-style option parsing entirely or do we need a version which outputs warnings first?

giladgray · 2018-06-26T19:27:19Z

src/language/rule/rule.ts

@@ -165,6 +165,8 @@ export interface ReplacementJson {
    innerText: string;
 }
 export class Replacement {
+    constructor(readonly start: number, readonly length: number, readonly text: string) {}


why are these constructors moved? i did not expect them to come first.

tslint.json: use preset "fields-first" for member-ordering rule instead of previous old-style config.
This seems to match previous config most closely, requiring only 5 changes.

I can recheck if one of the other presets matches more closely, otherwise, this seems the way to go...

Yeah this preset is the closest.

test/rules/member-ordering/alphabetize/test.ts.fix

src/rules/memberOrderingRule.ts

giladgray · 2018-06-26T19:36:22Z

tslint.json

-      "static-before-instance",
-      "variables-before-functions"
-    ],
+    "member-ordering": [true, { "order": "fields-first" }],


is the old options syntax still supported?

oh right, you asked. we cannot ship a breaking change. please preserve the old syntax, or we'll have to talk about 6.0

NaridaL · 2018-06-26T20:59:50Z

@giladgray I've added new categories for accessors. I've added these to the presets. (Currently getters and setters are ignored entirely). Therefore, if a user is using one of the presets, this is going to be a breaking change one way or the other. We might as well add other breaking changes (such as removing old style options).

giladgray · 2018-06-26T21:33:01Z

@NaridaL there's a big difference between a change in the lint result and an API change in the configuration. changing the configuration is a new API contract and requires user intervention during the upgrade in a way that is not automate-able.

conversely, changing the lint result may have no effect (if it's auto-fixable, which it is in this case), or it fails their build and they can easily update the code accordingly. we do not consider lint style changes to be API breaking.

so adding new categories is not an actual API break: it's actually an enhancement. but changing the config options in a non-backward-compatible way is an API break.

NaridaL · 2018-06-26T22:03:22Z

Ah, thanks for clearing up the distinction. I haven't removed the old- style option handling code, so this pr isn't breaking by your definition. I could add a console warning when using this option so that it could be removed in a future release

…

On Tue, Jun 26, 2018, 23:33 Gilad Gray ***@***.***> wrote: @NaridaL <https://github.com/NaridaL> there's a big difference between a change in the lint result and an *API change* in the configuration. changing the configuration is a new API contract and *requires* user intervention during the upgrade in a way that is not automate-able. conversely, changing the lint result may have no effect (if it's auto-fixable, which it is in this case), or it fails their build and they can easily update the code accordingly. we do not consider lint style changes to be API breaking. so adding new categories is not an actual API break: it's actually an enhancement. but changing the config options in a non-backward-compatible way is an API break. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3984 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKU8SvrzB_i8io705VTp7IYvdwSydNlNks5uAqiPgaJpZM4UyIXH> .

giladgray · 2018-06-27T19:15:18Z

no need to add a console warning as this would apply to all rules

NaridaL · 2018-06-27T19:38:34Z

@giladgray All rules?

I was thinking something like

giladgray · 2018-06-27T19:44:20Z

that is awesome! very clear message with migration steps 👌
however, definitely does not belong in this PR as that looks like a rather complex feature in itself and deserves to be reviewed separately.

don't think we're ready to deprecate old options usage just yet, though i imagine that'll be a flagship feature for 6.0 (that's what i meant by all rules -- migrating to object syntax across the board)

NaridaL · 2018-06-27T20:13:53Z

The rationale for adding the warning here is that the old style options don't handle accessors correctly and output unhelpful warnings. This rule already handles old style options by converting them to current ones, therefore outputting the above warning doesn't require any logic.

src/rules/memberOrderingRule.ts

NaridaL · 2018-07-05T15:23:56Z

Failing tests are due to latest commit on master.

maiis · 2018-07-18T11:00:51Z

@NaridaL I think you need to rebase this commit to fix the tests: 29fbace

… and setters. Added getters/setters to member-ordering/alphabetize/test.ts.lint

…ad of previous old-style config. This seems to match previous config most closely, requiring only 5 changes.

… locale/node config.

NaridaL · 2018-08-24T22:35:41Z

rebased onto master

ericanderson · 2018-12-12T16:26:03Z

I'm marking this as breaking due to the changed meaning of the presets.

JoshuaKGoldberg · 2019-10-02T07:37:27Z

Removing the breaking change label per #4811!

JoshuaKGoldberg

This LGTM. Will wait for review from @adidahiya for a week or so before merging, just in case.

adidahiya · 2019-10-05T19:28:32Z

thanks for the PR @NaridaL!

NaridaL mentioned this pull request Jun 26, 2018

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

Merged

4 tasks

giladgray reviewed Jun 26, 2018

View reviewed changes

giladgray changed the title ~~[bugfix] Fix #3965 getters and setters ignored by member-ordering-rule~~ [bugfix] member-ordering includes getters and setters Jun 26, 2018

NaridaL commented Jun 29, 2018

View reviewed changes

src/rules/memberOrderingRule.ts Show resolved Hide resolved

giladgray mentioned this pull request Aug 13, 2018

Getters and setters do not respect member ordering #4098

Closed

NaridaL added 7 commits August 25, 2018 00:28

Added "public-static-accessor" and others to correctly handle getters…

f5ebd18

… and setters. Added getters/setters to member-ordering/alphabetize/test.ts.lint

tslint.json: use preset "fields-first" for member-ordering rule inste…

090fb72

…ad of previous old-style config. This seems to match previous config most closely, requiring only 5 changes.

Fix switched private/public.

574b2f5

Handle get/set order properly.

33c7656

Output warning that old-style options do not handle getters.

79cdb39

Don't use localeCompare to avoid different orders depending on system…

4d7e6ef

… locale/node config.

Simplify checker slightly.

2ba99db

JoshuaKGoldberg added PR: waiting for reviewer PR: merge target branch labels Nov 7, 2018

ericanderson added the Type: Breaking Change label Dec 12, 2018

glen-84 mentioned this pull request Aug 31, 2019

[member-ordering] Order of getters and setters (accessors) typescript-eslint/typescript-eslint#929

Closed

JoshuaKGoldberg removed the Type: Breaking Change label Oct 2, 2019

Merge branch 'master'

b88e57e

JoshuaKGoldberg self-assigned this Oct 5, 2019

Re-ran prettier on memberOrderingRule.ts

fa8f31b

JoshuaKGoldberg removed PR: merge target branch PR: waiting for reviewer labels Oct 5, 2019

...but with the right Prettier version

0eb988c

JoshuaKGoldberg removed their assignment Oct 5, 2019

JoshuaKGoldberg approved these changes Oct 5, 2019

View reviewed changes

JoshuaKGoldberg added the PR: waiting for reviewer label Oct 5, 2019

adidahiya approved these changes Oct 5, 2019

View reviewed changes

adidahiya removed the PR: waiting for reviewer label Oct 5, 2019

adidahiya merged commit df68ccf into palantir:master Oct 5, 2019

adidahiya mentioned this pull request Nov 3, 2019

Prepare release v6.0.0-beta0 #4885

Merged

snebjorn mentioned this pull request Mar 25, 2020

tslint 6 compatibility mgechev/codelyzer#959

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

NaridaL commented Jun 21, 2018 •

edited by giladgray

Loading

giladgray commented Jun 26, 2018

NaridaL commented Jun 26, 2018

NaridaL commented Jun 26, 2018 •

edited

Loading

NaridaL commented Jun 26, 2018

giladgray Jun 26, 2018

NaridaL Jun 26, 2018

NaridaL Jun 27, 2018

giladgray Jun 26, 2018

giladgray Jun 26, 2018

NaridaL commented Jun 26, 2018

giladgray commented Jun 26, 2018

NaridaL commented Jun 26, 2018 via email

giladgray commented Jun 27, 2018

NaridaL commented Jun 27, 2018

giladgray commented Jun 27, 2018

NaridaL commented Jun 27, 2018

NaridaL commented Jul 5, 2018

maiis commented Jul 18, 2018

NaridaL commented Aug 24, 2018

ericanderson commented Dec 12, 2018

JoshuaKGoldberg commented Oct 2, 2019

JoshuaKGoldberg left a comment

adidahiya commented Oct 5, 2019

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

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

Conversation

NaridaL commented Jun 21, 2018 • edited by giladgray Loading

PR checklist

Overview of change:

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

CHANGELOG.md entry:

giladgray commented Jun 26, 2018

NaridaL commented Jun 26, 2018

NaridaL commented Jun 26, 2018 • edited Loading

NaridaL commented Jun 26, 2018

giladgray Jun 26, 2018

Choose a reason for hiding this comment

NaridaL Jun 26, 2018

Choose a reason for hiding this comment

NaridaL Jun 27, 2018

Choose a reason for hiding this comment

giladgray Jun 26, 2018

Choose a reason for hiding this comment

giladgray Jun 26, 2018

Choose a reason for hiding this comment

NaridaL commented Jun 26, 2018

giladgray commented Jun 26, 2018

NaridaL commented Jun 26, 2018 via email

giladgray commented Jun 27, 2018

NaridaL commented Jun 27, 2018

giladgray commented Jun 27, 2018

NaridaL commented Jun 27, 2018

NaridaL commented Jul 5, 2018

maiis commented Jul 18, 2018

NaridaL commented Aug 24, 2018

ericanderson commented Dec 12, 2018

JoshuaKGoldberg commented Oct 2, 2019

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

adidahiya commented Oct 5, 2019

NaridaL commented Jun 21, 2018 •

edited by giladgray

Loading

NaridaL commented Jun 26, 2018 •

edited

Loading