Conversation
| @@ -1,5 +1,4 @@ | |||
| // Generated by Grafana k6 Studio (0.0.0-vitest) on 2023-10-01T00:00:00.000Z | |||
There was a problem hiding this comment.
The header is arguably one place where we'd want there to be a space after.
There was a problem hiding this comment.
Pull request overview
This pull request refactors the code generation system to make comments "stick" to the statements below them by removing the blank line that was previously inserted. The change updates the NewLine type from a string union to an object with optional boolean or 'never' properties, allowing more granular control over newline behavior.
Changes:
- Modified the
NewLinetype from a string union to an object with optionalbeforeandafterproperties - Updated spacing utility functions to work with the new object-based NewLine type
- Changed the default behavior for comments to use
{ after: 'never' }, preventing newlines from being added after them - Updated 38 snapshot test files to reflect the removal of blank lines after comments
Reviewed changes
Copilot reviewed 34 out of 35 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/codegen/estree/typescript-estree.d.ts |
Refactored NewLine type from string union to object with before/after properties |
src/codegen/browser/formatting/spacing.ts |
Updated spacing functions to use new object-based NewLine type |
src/codegen/browser/formatting/formatter.ts |
Updated applySpacing to check object properties instead of string values |
src/codegen/browser/code/comment.ts |
Added default newLine parameter with { after: 'never' } to prevent spacing |
src/codegen/browser/__snapshots__/**/*.ts (38 files) |
Removed blank lines between generated comments and subsequent code |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { ...first, newLine: mergeNewLine(first.newLine, { before: true }) }, | ||
| ...middle, | ||
| { ...last, newLine: mergeNewLine(last.newLine, 'after') }, | ||
| { ...last, newLine: mergeNewLine(last.newLine, { after: false }) }, |
There was a problem hiding this comment.
This should be { after: true } not { after: false }. The spaceAround function is supposed to add spacing before the first element and after the last element, but this line sets after to false, which would prevent adding a newline after the last element. This is inconsistent with the function's intended behavior and with line 89 which correctly sets before to true.
| const before = | ||
| newLine.before !== undefined && target?.before === 'never' | ||
| ? target?.before | ||
| : newLine.before | ||
|
|
||
| const after = | ||
| newLine.after !== undefined && target?.after === 'never' | ||
| ? target?.after | ||
| : newLine.after | ||
|
|
||
| return 'both' | ||
| return { before, after } |
There was a problem hiding this comment.
The merging logic doesn't preserve target properties when newLine doesn't specify them. For example, if target has before: 'never' and newLine is { after: true } (with before undefined), the result will have before=undefined, losing the 'never' constraint. The logic should preserve target values when newLine doesn't explicitly override them. Consider using: const before = newLine.before !== undefined ? (target.before === 'never' ? 'never' : newLine.before) : target.before and similarly for after. This ensures that 'never' constraints are always respected, and target values are preserved when not explicitly overridden.
Llandy3d
left a comment
There was a problem hiding this comment.
I agree that for the header I would keep the space after, also it would be great to confirm that this doesn't break the checking we do in dashboards 🤔
@Llandy3d I've added back the space after the header. We're not emitting any other type of comment right now, so we don't need to worry about anything getting messed up in the dashboards. I did this PR to make the output nicer in this contributor PR. |
Description
In idiomatic code, comments usually stick to the statements below them. Our codegen was emitting a new line between a comment and the statement below it. This PR changes the behavior to make the comment stick to the element below it.
The API is configurable to opt-out of the "stickiness".
Before:
After:
How to Test
Checklist
Related PR(s)/Issue(s)
Related to #1037