Fix insertNodeAtClassStart for empty class with comment
#23342
Conversation
ghost
force-pushed
the
insertNodeAtClassStart
branch
from
44173aa
to
37d21b5
Compare
| export function addToSeen(seen: Map<true>, key: string | number): boolean { | ||
| export function addToSeen(seen: Map<true>, key: string | number): boolean; | ||
| export function addToSeen<T>(seen: Map<T>, key: string | number, value: T): boolean; | ||
| export function addToSeen<T>(seen: Map<T>, key: string | number, value: T = true as any): boolean { |
There was a problem hiding this comment.
For my own edification, why not use {} as the default value?
There was a problem hiding this comment.
I've been using Map<true> everywhere for sets; to me Map<{}> would imply actually being a map from string to something, where we didn't specify that something.
| if (addToSeen(this.classesWithNodesInsertedAtStart, getNodeId(cls), cls)) { | ||
| prefix = this.newLineCharacter; | ||
| // For `class C {\n}`, don't add the trailing "\n" | ||
| if (cls.members.length === 0 && !(positionsAreOnSameLine as any)(...getClassBraceEnds(cls, sourceFile), sourceFile)) { // TODO: GH#4130 remove 'as any' |
There was a problem hiding this comment.
Does the number of members matter? It seems like you want to omit the linebreak as long as the class is on a single line.
There was a problem hiding this comment.
It's the opposite -- if the class is on a single line I want to add line breaks on both sides to get {\n member\n}; if the class is already multiline I only want the first line break.
| } | ||
|
|
||
| const indentation = formatting.SmartIndenter.findFirstNonWhitespaceColumn(getLineStartPositionForPosition(clsStart, sourceFile), clsStart, sourceFile, this.formatContext.options) |
There was a problem hiding this comment.
Should the indentation be omitted if the class is on a single line?
There was a problem hiding this comment.
It's about to be multiline since we're adding a member, and that new member should be indented.
| this.classesWithNodesInsertedAtStart.forEach(cls => { | ||
| const sourceFile = cls.getSourceFile(); | ||
| const [openBraceEnd, closeBraceEnd] = getClassBraceEnds(cls, sourceFile); | ||
| // For `class C { }` remove the whitespace inside the braces. |
There was a problem hiding this comment.
- Why? I like having a space there.
- Shouldn't this be handled by the formatter (or at least consume the formatting options)?
There was a problem hiding this comment.
Leaving the space would result in the class ending in \n } instead of just \n} The formatter won't handle this since it only formats the new nodes we're adding, not the existing whitespace surrounding them.
|
@amcasey Could you update your review? |
Fixes #23333
Review #23343 before reviewing this