Skip to content

Better handle trailing commas with comments #126

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
willbeason wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Better handle trailing commas with comments #126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4c89382
Handle trailing commas with comments
willbeason Mar 19, 2026
35b3fd6
Keep comment spacing consistent
willbeason Mar 19, 2026
0a96ebb
Remove unnecessary function
willbeason Mar 19, 2026
a85ec57
Remove spurious IDE changes
willbeason Mar 19, 2026
9cb6680
Add example badly-handled comments
willbeason Mar 19, 2026
9c19ba4
Remove globals
willbeason Mar 19, 2026
ab7d555
Simplify implementation and document edge cases
willbeason Mar 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 19 additions & 7 deletions goldens/trailing_commas.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,25 +7,37 @@ Trailing comma except for last


Trailing comma and a comment
TODO: https://github.com/google/keep-sorted/issues/33 - Fix this
keep-sorted-test start
3, # three
1,
2
keep-sorted-test end

Trailing comma except for last, last has a comment
TODO: https://github.com/google/keep-sorted/issues/33 - Fix this
keep-sorted-test start
3,
1,
2 # two
1, <!-- one -->
2 <!-- two -->
keep-sorted-test end

Trailing comma and a comment, last has a comment
TODO: https://github.com/google/keep-sorted/issues/33 - Fix this
keep-sorted-test start
3, # three
3, -- three
1,
2 # two
2 -- two
keep-sorted-test end

Trailing commas with inline comment
keep-sorted-test start
"c" /* c */,
"b",
"a" /* a */
keep-sorted-test end

Trailing commas with quoted comment characters
TODO: https://github.com/google/keep-sorted/issues/33 - Fix this
keep-sorted-test start
"c",
"b, //",
Copy link
Copy Markdown
Collaborator

@JeffFaer JeffFaer Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

block=yes has some logic that looks at quotes and tries to understand whether we're in a quoted section or not:

keep-sorted/keepsorted/line_group.go

Line 354 in e2a9a7f

if cb.expectedQuote == "" {

I wonder if we could leverage that here to exclude comment leaders that are actually part of a string constant

Per my other comment, that code should probably use StickyPrefixes instead of just commentMarker, but that's again me just thinking out loud

"a, #"
keep-sorted-test end
30 changes: 21 additions & 9 deletions goldens/trailing_commas.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,25 +7,37 @@ Trailing comma except for last


Trailing comma and a comment
TODO: https://github.com/google/keep-sorted/issues/33 - Fix this
keep-sorted-test start
1,
2
3, # three
2,
3 # three
keep-sorted-test end

Trailing comma except for last, last has a comment
Copy link
Copy Markdown
Contributor Author

@willbeason willbeason Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the inconsistent spacing here. I played with an approach that made spacing consistently one space after the comma, but it was a bit more complex. I have this behavior in the PR since the implementation is simpler.

Copy link
Copy Markdown
Collaborator

@JeffFaer JeffFaer Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, yeah, that's weird. I'd somewhat prefer if we kept the spacing the same as what the user wrote originally. Oh I see, the .in file had two spaces to keep the comments aligned, and then we added a comma which bumped it out by one.

Maybe we should replace a single space with a comma, but if the comment only has a single leading space, preserve that space?

TODO: https://github.com/google/keep-sorted/issues/33 - Fix this
keep-sorted-test start
1,
2 # two,
1, <!-- one -->
2, <!-- two -->
3
keep-sorted-test end

Trailing comma and a comment, last has a comment
TODO: https://github.com/google/keep-sorted/issues/33 - Fix this
keep-sorted-test start
1,
2 # two
3, # three
2, -- two
3 -- three
keep-sorted-test end

Trailing commas with inline comment
keep-sorted-test start
"a", /* a */
"b",
"c" /* c */
keep-sorted-test end

Trailing commas with quoted comment characters
TODO: https://github.com/google/keep-sorted/issues/33 - Fix this
keep-sorted-test start
"a, #"
"b, //",
"c",
keep-sorted-test end
21 changes: 16 additions & 5 deletions keepsorted/block.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package keepsorted

import (
"fmt"
"regexp"
"slices"
"strings"

Expand Down Expand Up @@ -389,13 +390,23 @@ func handleTrailingComma(lgs []*lineGroup) (trimTrailingComma func([]*lineGroup)
}
}

if n := len(dataGroups); n > 1 && allHaveSuffix(dataGroups[0:n-1], ",") && !dataGroups[n-1].hasSuffix(",") {
dataGroups[n-1].append(",")
knownCommentMarkersList := knownCommentMarkers()
Copy link
Copy Markdown
Collaborator

@JeffFaer JeffFaer Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

block.metadata.opts records which comment marker is used in the keep-sorted start directive. That's what we use for sticky_comments and that can be expanded with sticky_prefixes (and that commentMarker is automatically added to the list of sticky_prefixes assuming sticky_comments is true)

What do you think about using block.metadata.opts.StickyPrefixes for this instead of knownCommentMarkers()? That gives users a lever they can use to configure this behavior

for i, marker := range knownCommentMarkersList {
// Some comment markers, such as "/*", include regex metacharacters.
knownCommentMarkersList[i] = regexp.QuoteMeta(marker)
}
knownCommentMarkersStr := strings.Join(knownCommentMarkersList, "|")

commaLineEnd := regexp.MustCompile(fmt.Sprintf(",(\\s*(%s).*)?$", knownCommentMarkersStr))
Copy link
Copy Markdown
Collaborator

@JeffFaer JeffFaer Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use grave ticks so you don't need to escape the backslash

lineEnd := regexp.MustCompile(fmt.Sprintf("(\\s*(%s).*)?$", knownCommentMarkersStr))

if n := len(dataGroups); n > 1 && allMatchSuffix(dataGroups[0:n-1], commaLineEnd) && !dataGroups[n-1].matchesSuffix(commaLineEnd) {
dataGroups[n-1].replaceSuffix(lineEnd, ",$1")
Copy link
Copy Markdown
Collaborator

@JeffFaer JeffFaer Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you happen to know how nested capturing groups behave in go? Is $1 referencing (\\s*(%s).*) or is it referencing just (%s)?

Either way, we only seem to be using $1, so consider using a non-capturing group for the other set of parens so that you don't need to answer this question ;)


return func(lgs []*lineGroup) {
for i := len(lgs) - 1; i >= 0; i-- {
if len(lgs[i].lines) > 0 {
lgs[i].trimSuffix(",")
lgs[i].replaceSuffix(commaLineEnd, "$1")
Copy link
Copy Markdown
Collaborator

@JeffFaer JeffFaer Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this why the spacing seems weird? Should we replace this with " $1" so that the comma is essentially replaced with a space?

return
}
}
Expand All @@ -405,9 +416,9 @@ func handleTrailingComma(lgs []*lineGroup) (trimTrailingComma func([]*lineGroup)
return func([]*lineGroup) {}
}

func allHaveSuffix(lgs []*lineGroup, s string) bool {
func allMatchSuffix(lgs []*lineGroup, suffix *regexp.Regexp) bool {
for _, lg := range lgs {
if !lg.hasSuffix(s) {
if !lg.matchesSuffix(suffix) {
return false
}
}
Expand Down
15 changes: 15 additions & 0 deletions keepsorted/keep_sorted_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1126,6 +1126,21 @@ func TestLineSorting(t *testing.T) {
"foo",
},
},
{
name: "TrailingCommas_WithComments",

in: []string{
"foo,",
"baz, /* baz */",
"bar // bar",
},

want: []string{
"bar, // bar",
"baz, /* baz */",
"foo",
},
},
{
name: "IgnorePrefixes",

Expand Down
11 changes: 10 additions & 1 deletion keepsorted/line_group.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ func groupLines(lines []string, metadata blockMetadata) []*lineGroup {
// Returns another boolean indicating whether the group should be ending
// after that line if so.
shouldAddToRegexDelimitedGroup := func(l string) (addToGroup bool, finishGroupAfter bool) {
if metadata.opts.GroupStartRegex != nil {
if metadata.opts.GroupStartRegex != nil {
// For GroupStartRegex, all non-regex-matching lines should be
// part of the group including prior lines.
return !matchesAnyRegex(l, metadata.opts.GroupStartRegex), false
Expand Down Expand Up @@ -408,11 +408,20 @@ func (lg *lineGroup) hasSuffix(s string) bool {
return len(lg.lines) > 0 && strings.HasSuffix(lg.lines[len(lg.lines)-1], s)
}

func (lg *lineGroup) matchesSuffix(re *regexp.Regexp) bool {
return len(lg.lines) > 0 && re.MatchString(lg.lines[len(lg.lines)-1])
}

func (lg *lineGroup) trimSuffix(s string) {
lg.access = accessRecorder{}
lg.lines[len(lg.lines)-1] = strings.TrimSuffix(lg.lines[len(lg.lines)-1], s)
}

func (lg *lineGroup) replaceSuffix(re *regexp.Regexp, replacement string) {
lg.access = accessRecorder{}
lg.lines[len(lg.lines)-1] = re.ReplaceAllString(lg.lines[len(lg.lines)-1], replacement)
}

func (lg *lineGroup) commentOnly() bool {
lg.access.commentOnly = true
return len(lg.lines) == 0
Expand Down
6 changes: 5 additions & 1 deletion keepsorted/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,9 +312,13 @@ func formatIntList(intVals []int) string {
return strings.Join(vals, ",")
}

func knownCommentMarkers() []string {
return []string{"//", "#", "/*", "--", ";", "<!--"}
}

func guessCommentMarker(startLine string) string {
startLine = strings.TrimSpace(startLine)
for _, marker := range []string{"//", "#", "/*", "--", ";", "<!--"} {
for _, marker := range knownCommentMarkers() {
if strings.HasPrefix(startLine, marker) {
return marker
}
Expand Down
Loading