[bugfix] member-ordering includes getters and setters#3984
Conversation
|
Done. Let's see if the lint still fails... |
|
OK, yeah, for some reason running 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. |
|
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? |
| innerText: string; | ||
| } | ||
| export class Replacement { | ||
| constructor(readonly start: number, readonly length: number, readonly text: string) {} |
There was a problem hiding this comment.
why are these constructors moved? i did not expect them to come first.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
Yeah this preset is the closest.
| "static-before-instance", | ||
| "variables-before-functions" | ||
| ], | ||
| "member-ordering": [true, { "order": "fields-first" }], |
There was a problem hiding this comment.
is the old options syntax still supported?
There was a problem hiding this comment.
oh right, you asked. we cannot ship a breaking change. please preserve the old syntax, or we'll have to talk about 6.0
|
@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). |
|
@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. |
giladgray
changed the title
|
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>
.
|
|
no need to add a console warning as this would apply to all rules |
|
@giladgray All rules? I was thinking something like
|
|
that is awesome! very clear message with migration steps 👌 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) |
|
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. |
|
Failing tests are due to latest commit on master. |
… 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.
|
rebased onto master |
|
I'm marking this as breaking due to the changed meaning of the presets. |
|
Removing the |
JoshuaKGoldberg
left a comment
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
This LGTM. Will wait for review from @adidahiya for a week or so before merging, just in case.
|
thanks for the PR @NaridaL! |
6 participants
PR checklist
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-orderingincludes property accessors (getters and setters).