[@typescript-eslint/member-ordering] auto-fix #2296
Comments
|
Sounds like a good idea, but dealing with things like comments makes this a very difficult problem to solve. Leading docblocks are mostly simple, but we'd have to design some logic to decide which comments to ignore and which ones are notclass Foo {
/**
Double star comments are probably safe to always attach
*/
prop = 1;
/*
Is a single star comment attached?
*/
prop2 = 1;
/** What about if there's two comments? */
/** Do we just attach one or both? */
prop3 = 1;
/**
What about this case where the comment is miles away? Is this attached?
*/
prop4 = 1;
prop5 = 1; /* what about trailing comments? Have to make sure they're included */
method1() {
return 1;
} // trailing comments will be a pain in the ass to handle
}What about single line comments?class Foo {
// should this be attached?
prop = 1;
// what about
// multiline comments?
prop2 = 1;
// What about this case where the comment is miles away? Is this attached?
prop4 = 1;
// it's not uncommon to use comments to separate blocks
// so that you can add some delimiting
// what happens to these comments?
prop5 = 1;
prop6 = 1; // what about trailing comments? Have to make sure they're included
method1() {
return 1;
} // trailing comments will be a pain in the ass to handle
}Then you get into stupid but completely valid cases that should really be handled like this...// What should the "fixed" form look like?
class B {
/* 1 */ single() {} /* 2 */ line() {} /* 3 */ declaration() {} // 4
}I think this is a nice idea in theory, but it's really hard to implement well and cover all of the cases - I'm not sure if we really want the maintenance overhead with this? |
|
What if comments that aren't directly above the line are ignored and it's opt-in? For auto-generated declarations this would greatly improve the readability of commits where currently it's not unusual to have massive changes due to the order of interface properties changing. |
|
Would be interesting to see how this was solved in tslint which does support auto-fixing: https://palantir.github.io//tslint/rules/member-ordering/ |
|
Unsure. I've never worked on tslint rules, so IDK how their fixers work exactly. It looks like they just handle directly leading and directly trailing comments? Unsure exactly as their tests aren't the most comprehensive. If someone wants to attempt to add this, happy to accept a PR. |
|
Hey, what about auto fix when there are no or only in-line comments? |
|
It can be assumed programmatically to move a method/property with all the comments and decorators that are defined above it or trailing (on the right side). People would never place a comment below a method/property and if it has comments then in all cases that @bradzacher described it'd be fine to take the comments above the method/property with it when moving it. |
|
Any news on this? |
|
With any issue in opened in this project - it either has a visible progress in the form of an attached PR, or it has no progress. We are a community run project. The volunteer maintainers spend most of their time triaging issues and reviewing PRs. This means that most issues will not progress unless a member of the community steps up and champions it. If this issue is important to you - consider being that champion. If not - please just subscribe to the issue and wait patiently. |
|
A VS Code extension Typescript Class Organizer (repo) is available to order members in a class. |
|
It would be great if @aljazsim was available to champion this issue |
|
This feature is long awaited even if there would be a problem with comments, which I am convinced, can be solved e.g. by keeping it together with the nearest class member. |
|
We are a community run project. The volunteer maintainers spend most of their time triaging issues and reviewing PRs. This means that most issues will not progress unless a member of the community steps up and champions it. If this issue is important to you - consider being that champion. |
|
it used to have fixer in tslint, but now it's just a warning |
it has serious bug and author is not in favor of fixing them so i'm not sure if that's a good solution |
|
So the fastest way to resolve this is probably to put a PR together for Typescript Class Organizer if @aljazsim is willing to merge. |
|
Helpful info for a possible PR: |
|
FYI, I stumbled upon eslint-plugin-typescript-sort-keys and it seems to be what I was looking for. |
|
@bradzacher nice analyze regarding the comments issue. A. Any comment (line or beginning of block) preceding a class member belongs to this class member until another class member or an empty (blank) line is found. class Foo {
/**
Attached
*/
prop = 1;
/*
Attached
*/
prop2 = 1;
/** Attached */
/** Also attached */
prop3 = 1;
/**
Not attached as separated by an empty line
*/
/** But attached */
prop4 = 1;
prop5 = 1; /* Attached to prop5 and not method1 because the comment is on the class member's terminator ";" same line */
method1() {
return 1;
} // attached to method1() because the comment is on the same line as method1() terminator "}".
}Single line comments class Foo {
// attached to prop
prop = 1;
// As there is no blank line, this also attached to prop2
// attached to prop2
prop2 = 1;
// I'm not attached to prop4 because there is a blank line preceding prop4
prop4 = 1;
// Those are not attached either to prop5
// like imports are grouped when they are separated by a blank line
// adding a blank line detach the comments from the class member
prop5 = 1;
prop6 = 1; // this belongs to prop6 because it follows the class member's terminator ";"
method1() {
return 1;
} // this belongs to method1 because it's it follows the class member's terminator "}"
}And the weird one // What should the "fixed" form look like?
class B {
/* attached to single() */ single() {} /* trailing of single but attached to line() */ line() {} /* trailing of line() but attached to declaration() */ declaration() {} // trailing of declaration
}To summarize the ambiguous case: class B {
/*I am attached to prop1*/ prop1: string /*I am attached to prop1 too, because there is no other class member on this line*/
/*I am attached to prop2*/ prop2: string /*I am attached to prop3 */ prop3: string;
/*I am attached to prop4*/ prop4: string
/*I am not attached to any class member */;
}But also… if people are formatting their code properly, using prettier for instance… there should be no such ambiguity. |
|
I upvote, please add this! It's a nightmare when you have big files and you'd like them to be ordered. |
|
Is there a way for auto-fix logic to be implemented only in "basic" cases (only doc comments, only single line comments (assumed attached to next value), no comments, etc)? In other words, is there a solution that delivers some value and removes manual effort even if not the perfect solution? Note: Maybe y’all have tenets around DX, but I wonder if there’s incremental value to deliver here. |
|
In theory, yes. The ...but in practice users hate this - we'll end up getting lots of bug reports from people saying the fixer breaks / doesn't work in each and every one of those non-basic cases. 😞 |
I can understand that. Thanks for the reply! |
|
Maybe, this lands on Prettier scope. I'll be checking these packages: |
|
I would argue that the debate around which comments to ignore is unnecessary. AFAICT the only reason you would ignore comments is in instances when someone has created headers/delimiters for sub-sections of the members to create some sort of ordering AND you wanted to somehow continue to support that. But in activating this rule, you are explicitly opting out of any existing manual organization, rendering these headers obsolete. Under that assumption, you can safely assume that all comments between member nodes are tied to the following node which frees up a simple re-ordering logic. Expanding that with support for trailing comments being associated with the object on their line and all cases are addressed. This is basically a simplifying expansion of @Xample's logic: This results in full fluidity within the sort-space as well, simplifying logic and thus maintenance burden. And for support, the fixer just does not support alternate sortings/groupings tautologically which should eliminate the need for arbitrary headers. |
|
Given fixing, as has been pointed out, is potentially problematic to automatically fix, could this instead be provided via a suggestion? |
That would only exacerbate this:
Plus, this rule would still complain and force you to do manual work for no good reason when it could just auto-fix it. Recently, TypeScript Class Organizer have been good enough for my usage. It even handles comments. It has some stupid options enabled by default, but that can be easily fixed with a few settings, namely |
|
I published prettier-plugin-sort-members as #2296 (comment) suggests 🚀 Edit: |
|
"Good" timing! Per #8792, we're feature freezing this rule. It's reached the point of complexity where we're having a hard time justifying working on it. If you'd like to see this option happen, I'd encourage you to add it to a community project such as eslint.style or eslint-plugin-per D109 fectionist. Cheers! 💙 |
|
I also mentioned this just now over in my question to the ESLint Stylistic maintainers about potentially migrating this rule to ESLint Stylistic: |
Repro
Expected Result
eslint --fixsorts members alphabeticallyActual Result
Throws warnings but doesn't fix.
Additional Info
Versions
@typescript-eslint/eslint-plugin3.6.1@typescript-eslint/parser3.6.1TypeScript3.8.3ESLint6.8.0node12.16.2npm6.14.5The text was updated successfully, but these errors were encountered: