From 2113765a3648437e5087cc3b35183447a4be5a23 Mon Sep 17 00:00:00 2001 From: Jeffrey Faer Date: Thu, 4 Apr 2024 15:04:47 -0600 Subject: [PATCH 1/4] refactor(options): Define default options as an explicit instance. This simplifies things a bit, and sets us up to add a flag to override the default options. --- keepsorted/block.go | 2 +- keepsorted/keep_sorted_test.go | 391 ++++++++++++++++----------------- keepsorted/options.go | 83 +++---- 3 files changed, 240 insertions(+), 236 deletions(-) diff --git a/keepsorted/block.go b/keepsorted/block.go index 65e8716..c292c85 100644 --- a/keepsorted/block.go +++ b/keepsorted/block.go @@ -109,7 +109,7 @@ func (f *Fixer) newBlocks(lines []string, offset int, include func(start, end in continue } - opts, err := f.parseBlockOptions(start.line) + opts, err := f.parseBlockOptions(start.line, defaultOptions) if err != nil { // TODO(b/250608236): Is there a better way to surface this error? log.Err(fmt.Errorf("keep-sorted block at index %d had bad start directive: %w", start.index+offset, err)).Msg("") diff --git a/keepsorted/keep_sorted_test.go b/keepsorted/keep_sorted_test.go index 94a89e2..307030f 100644 --- a/keepsorted/keep_sorted_test.go +++ b/keepsorted/keep_sorted_test.go @@ -23,31 +23,6 @@ import ( "github.com/rs/zerolog/log" ) -var ( - defaultOptions = blockOptions{ - Lint: true, - SkipLines: 0, - Group: true, - GroupPrefixes: nil, - IgnorePrefixes: nil, - Numeric: false, - StickyComments: true, - StickyPrefixes: map[string]bool{"//": true}, - RemoveDuplicates: true, - PrefixOrder: nil, - Block: false, - NewlineSeparated: false, - CaseSensitive: true, - commentMarker: "//", - } - - defaultMetadata = blockMetadata{ - startDirective: "keep-sorted-test start", - endDirective: "keep-sorted-test end", - opts: defaultOptions, - } -) - // initZerolog initializes zerolog to log as part of the test. // It returns a function that restores zerolog to its state before this function was called. func initZerolog(t testing.TB) { @@ -56,16 +31,26 @@ func initZerolog(t testing.TB) { t.Cleanup(func() { log.Logger = oldLogger }) } -func defaultOptionsWith(f func(*blockOptions)) blockOptions { +func defaultOptionsWithCommentMarker(marker string) blockOptions { opts := defaultOptions - f(&opts) + opts.StickyComments = true + opts.setCommentMarker(marker) + return opts +} + +func optionsWithCommentMarker(marker string) blockOptions { + var opts blockOptions + opts.StickyComments = true + opts.setCommentMarker(marker) return opts } func defaultMetadataWith(opts blockOptions) blockMetadata { - meta := defaultMetadata - meta.opts = opts - return meta + return blockMetadata{ + startDirective: "keep-sorted-test start", + endDirective: "keep-sorted-test end", + opts: opts, + } } func TestFix(t *testing.T) { @@ -375,7 +360,7 @@ cat`, wantBlocks: []block{ { - metadata: defaultMetadata, + metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), start: 3, end: 7, lines: []string{ @@ -385,7 +370,7 @@ cat`, }, }, { - metadata: defaultMetadata, + metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), start: 9, end: 13, lines: []string{ @@ -412,7 +397,7 @@ dog wantBlocks: []block{ { - metadata: defaultMetadata, + metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), start: 5, end: 7, lines: []string{ @@ -450,7 +435,7 @@ cat`, wantBlocks: []block{ { - metadata: defaultMetadata, + metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), start: 3, end: 7, lines: []string{ @@ -478,7 +463,7 @@ cat`, wantBlocks: []block{ { - metadata: defaultMetadata, + metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), start: 1, end: 6, lines: []string{ @@ -511,7 +496,7 @@ i wantBlocks: []block{ { - metadata: defaultMetadata, + metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), start: 1, end: 13, lines: []string{ @@ -529,7 +514,7 @@ i }, nestedBlocks: []block{ { - metadata: defaultMetadata, + metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), start: 5, end: 9, lines: []string{ @@ -589,7 +574,7 @@ i wantBlocks: []block{ { - metadata: defaultMetadata, + metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), start: 1, end: 34, lines: []string{ @@ -628,7 +613,7 @@ i }, nestedBlocks: []block{ { - metadata: defaultMetadata, + metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), start: 5, end: 30, lines: []string{ @@ -659,7 +644,7 @@ i }, nestedBlocks: []block{ { - metadata: defaultMetadata, + metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), start: 9, end: 21, lines: []string{ @@ -677,7 +662,7 @@ i }, nestedBlocks: []block{ { - metadata: defaultMetadata, + metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), start: 13, end: 17, lines: []string{ @@ -689,7 +674,7 @@ i }, }, { - metadata: defaultMetadata, + metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), start: 22, end: 26, lines: []string{ @@ -703,7 +688,7 @@ i }, }, { - metadata: defaultMetadata, + metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), start: 35, end: 39, lines: []string{ @@ -729,7 +714,7 @@ i wantBlocks: []block{ { - metadata: defaultMetadata, + metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), start: 5, end: 7, lines: []string{"2"}, @@ -771,8 +756,7 @@ func TestLineSorting(t *testing.T) { { name: "NothingToSort", - opts: defaultOptions, - in: []string{}, + in: []string{}, want: []string{}, wantAlreadySorted: true, @@ -780,7 +764,6 @@ func TestLineSorting(t *testing.T) { { name: "AlreadySorted", - opts: defaultOptions, in: []string{ "Bar", "Baz", @@ -799,7 +782,9 @@ func TestLineSorting(t *testing.T) { { name: "AlreadySorted_ExceptForDuplicate", - opts: defaultOptions, + opts: blockOptions{ + RemoveDuplicates: true, + }, in: []string{ "Bar", "Bar", @@ -815,9 +800,9 @@ func TestLineSorting(t *testing.T) { { name: "AlreadySorted_NewlineSeparated", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.NewlineSeparated = true - }), + opts: blockOptions{ + NewlineSeparated: true, + }, in: []string{ "Bar", "", @@ -838,9 +823,9 @@ func TestLineSorting(t *testing.T) { { name: "AlreadySorted_ExceptForNewlineSorted", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.NewlineSeparated = true - }), + opts: blockOptions{ + NewlineSeparated: true, + }, in: []string{ "Bar", "Baz", @@ -859,7 +844,6 @@ func TestLineSorting(t *testing.T) { { name: "SimpleSorting", - opts: defaultOptions, in: []string{ "Baz", "Foo", @@ -877,10 +861,7 @@ func TestLineSorting(t *testing.T) { { name: "CommentOnlyBlock", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.StickyComments = true - opts.StickyPrefixes = map[string]bool{"//": true} - }), + opts: optionsWithCommentMarker("//"), in: []string{ "2", "1", @@ -896,9 +877,9 @@ func TestLineSorting(t *testing.T) { { name: "Prefix", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.PrefixOrder = []string{"INIT_", "", "FINAL_"} - }), + opts: blockOptions{ + PrefixOrder: []string{"INIT_", "", "FINAL_"}, + }, in: []string{ // keep-sorted start prefix_order= "DO_SOMETHING_WITH_BAR", @@ -937,11 +918,11 @@ func TestLineSorting(t *testing.T) { { name: "RemoveDuplicates_ConsidersComments", - opts: defaultOptionsWith(func(opts *blockOptions) { + opts: func() blockOptions { + opts := optionsWithCommentMarker("//") opts.RemoveDuplicates = true - opts.StickyComments = true - opts.StickyPrefixes = map[string]bool{"//": true} - }), + return opts + }(), in: []string{ "// comment 1", "foo", @@ -963,9 +944,9 @@ func TestLineSorting(t *testing.T) { { name: "RemoveDuplicates_IgnoresTraliningCommas", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.RemoveDuplicates = true - }), + opts: blockOptions{ + RemoveDuplicates: true, + }, in: []string{ "foo,", "bar,", @@ -980,9 +961,9 @@ func TestLineSorting(t *testing.T) { { name: "RemoveDuplicates_IgnoresTrailingCommas_RemovesCommaIfLastElement", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.RemoveDuplicates = true - }), + opts: blockOptions{ + RemoveDuplicates: true, + }, in: []string{ "foo,", "foo,", @@ -997,9 +978,9 @@ func TestLineSorting(t *testing.T) { { name: "RemoveDuplicates_IgnoresTrailingCommas_RemovesCommaIfOnlyElement", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.RemoveDuplicates = true - }), + opts: blockOptions{ + RemoveDuplicates: true, + }, in: []string{ "foo,", "foo", @@ -1012,9 +993,9 @@ func TestLineSorting(t *testing.T) { { name: "RemoveDuplicates_Keep", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.RemoveDuplicates = false - }), + opts: blockOptions{ + RemoveDuplicates: false, + }, in: []string{ "foo", "foo", @@ -1030,7 +1011,6 @@ func TestLineSorting(t *testing.T) { { name: "TrailingCommas", - opts: defaultOptions, in: []string{ "foo,", "baz,", @@ -1046,9 +1026,9 @@ func TestLineSorting(t *testing.T) { { name: "IgnorePrefixes", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.IgnorePrefixes = []string{"fs.setBoolFlag", "fs.setIntFlag"} - }), + opts: blockOptions{ + IgnorePrefixes: []string{"fs.setBoolFlag", "fs.setIntFlag"}, + }, in: []string{ // keep-sorted start ignore_prefixes= `fs.setBoolFlag("paws_with_cute_toebeans", true)`, @@ -1066,9 +1046,9 @@ func TestLineSorting(t *testing.T) { { name: "CaseInsensitive", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.CaseSensitive = false - }), + opts: blockOptions{ + CaseSensitive: false, + }, in: []string{ // keep-sorted start case=yes "Bravo", @@ -1086,9 +1066,9 @@ func TestLineSorting(t *testing.T) { { name: "Numeric", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.Numeric = true - }), + opts: blockOptions{ + Numeric: true, + }, in: []string{ // keep-sorted start numeric=no "PROGRESS_100_PERCENT", @@ -1110,10 +1090,10 @@ func TestLineSorting(t *testing.T) { { name: "MultipleTransforms", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.IgnorePrefixes = []string{"R2D2", "C3PO", "R4"} - opts.Numeric = true - }), + opts: blockOptions{ + IgnorePrefixes: []string{"R2D2", "C3PO", "R4"}, + Numeric: true, + }, in: []string{ // keep-sorted start ignore_prefixes= numeric=no "C3PO_ARM_L", @@ -1139,9 +1119,9 @@ func TestLineSorting(t *testing.T) { { name: "NewlineSeparated", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.NewlineSeparated = true - }), + opts: blockOptions{ + NewlineSeparated: true, + }, in: []string{ "B", "", @@ -1160,9 +1140,9 @@ func TestLineSorting(t *testing.T) { { name: "NewlineSeparated_Empty", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.NewlineSeparated = true - }), + opts: blockOptions{ + NewlineSeparated: true, + }, in: []string{}, want: []string{}, @@ -1192,7 +1172,6 @@ func TestLineGrouping(t *testing.T) { }{ { name: "Simple", - opts: defaultOptions, want: []lineGroup{ {nil, []string{"foo"}}, @@ -1201,10 +1180,7 @@ func TestLineGrouping(t *testing.T) { }, { name: "StickyComments", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.StickyComments = true - opts.StickyPrefixes = map[string]bool{"//": true} - }), + opts: optionsWithCommentMarker("//"), want: []lineGroup{ { @@ -1227,10 +1203,7 @@ func TestLineGrouping(t *testing.T) { }, { name: "CommentOnlyGroup", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.StickyComments = true - opts.StickyPrefixes = map[string]bool{"//": true} - }), + opts: optionsWithCommentMarker("//"), want: []lineGroup{ { @@ -1251,10 +1224,9 @@ func TestLineGrouping(t *testing.T) { }, { name: "Group", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.Group = true - opts.Block = false - }), + opts: blockOptions{ + Group: true, + }, want: []lineGroup{ {nil, []string{ @@ -1268,10 +1240,10 @@ func TestLineGrouping(t *testing.T) { }, { name: "Group_Prefixes", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.GroupPrefixes = map[string]bool{"and": true, "with": true} - opts.Block = false - }), + opts: blockOptions{ + Group: true, + GroupPrefixes: map[string]bool{"and": true, "with": true}, + }, want: []lineGroup{ {nil, []string{ @@ -1295,10 +1267,9 @@ func TestLineGrouping(t *testing.T) { }, { name: "Group_UnindentedNewlines", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.Group = true - opts.Block = false - }), + opts: blockOptions{ + Group: true, + }, want: []lineGroup{ {nil, []string{ @@ -1319,10 +1290,11 @@ func TestLineGrouping(t *testing.T) { }, { name: "Group_NestedKeepSortedBlocksWithoutAnyIndentation", - opts: defaultOptionsWith(func(opts *blockOptions) { + opts: func() blockOptions { + opts := optionsWithCommentMarker("//") opts.Group = true - opts.Block = false - }), + return opts + }(), want: []lineGroup{ {[]string{ @@ -1347,9 +1319,9 @@ func TestLineGrouping(t *testing.T) { }, { name: "Block_Brackets", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.Block = true - }), + opts: blockOptions{ + Block: true, + }, want: []lineGroup{ {nil, []string{ @@ -1368,9 +1340,9 @@ func TestLineGrouping(t *testing.T) { }, { name: "Block_Quotes", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.Block = true - }), + opts: blockOptions{ + Block: true, + }, want: []lineGroup{ {nil, []string{ @@ -1389,9 +1361,9 @@ func TestLineGrouping(t *testing.T) { }, { name: "Block_EscapedQuote", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.Block = true - }), + opts: blockOptions{ + Block: true, + }, want: []lineGroup{ {nil, []string{ @@ -1410,9 +1382,9 @@ func TestLineGrouping(t *testing.T) { }, { name: "Block_IgnoresQuotesWithinQuotes", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.Block = true - }), + opts: blockOptions{ + Block: true, + }, want: []lineGroup{ {nil, []string{ @@ -1431,9 +1403,9 @@ func TestLineGrouping(t *testing.T) { }, { name: "Block_IgnoresBracesWithinQuotes", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.Block = true - }), + opts: blockOptions{ + Block: true, + }, want: []lineGroup{ {nil, []string{ @@ -1452,10 +1424,11 @@ func TestLineGrouping(t *testing.T) { }, { name: "Block_IgnoresSpecialCharactersWithinFullLineComments", - opts: defaultOptionsWith(func(opts *blockOptions) { + opts: func() blockOptions { + opts := optionsWithCommentMarker("//") opts.Block = true - opts.StickyPrefixes["//"] = true - }), + return opts + }(), want: []lineGroup{ {nil, []string{ @@ -1476,10 +1449,11 @@ func TestLineGrouping(t *testing.T) { }, { name: "Block_IgnoresSpecialCharactersWithinTrailingComments", - opts: defaultOptionsWith(func(opts *blockOptions) { + opts: func() blockOptions { + opts := optionsWithCommentMarker("//") opts.Block = true - opts.StickyPrefixes["//"] = true - }), + return opts + }(), want: []lineGroup{ {nil, []string{ @@ -1502,9 +1476,9 @@ func TestLineGrouping(t *testing.T) { }, { name: "Block_TripleQuotes", - opts: defaultOptionsWith(func(opts *blockOptions) { - opts.Block = true - }), + opts: blockOptions{ + Block: true, + }, want: []lineGroup{ {nil, []string{ @@ -1534,106 +1508,127 @@ func TestLineGrouping(t *testing.T) { func TestBlockOptions(t *testing.T) { for _, tc := range []struct { - name string - in string + name string + in string + defaultOptions blockOptions want blockOptions wantErr string }{ { - name: "DefaultOptions", - in: "// keep-sorted-test", + name: "DefaultOptions", + in: "", + defaultOptions: defaultOptions, want: defaultOptions, }, + { + name: "CommentMarker", + in: "// keep-sorted-test start", + + want: blockOptions{ + commentMarker: "//", + }, + }, + { + name: "StickyComments", + in: "// keep-sorted-test start sticky_comments=yes", + + want: blockOptions{ + StickyComments: true, + StickyPrefixes: map[string]bool{"//": true}, + commentMarker: "//", + }, + }, { name: "SimpleSwitch", - in: "// keep-sorted-test lint=no", + in: "lint=yes", - want: defaultOptionsWith(func(opts *blockOptions) { - opts.Lint = false - }), + want: blockOptions{Lint: true}, }, { name: "SkipLines", - in: "// keep-sorted-test skip_lines=10", + in: "skip_lines=10", - want: defaultOptionsWith(func(opts *blockOptions) { - opts.SkipLines = 10 - }), + want: blockOptions{SkipLines: 10}, }, { name: "ErrorSkipLinesIsNegative", - in: "// keep-sorted-test skip_lines=-1", + in: "skip_lines=-1", - want: defaultOptions, - wantErr: `skip_lines has invalid value: -1`, + wantErr: "skip_lines has invalid value: -1", }, { name: "ItemList", - in: "// keep-sorted-test prefix_order=a,b,c,d", + in: "prefix_order=a,b,c,d", - want: defaultOptionsWith(func(opts *blockOptions) { - opts.PrefixOrder = []string{"a", "b", "c", "d"} - }), + want: blockOptions{ + PrefixOrder: []string{"a", "b", "c", "d"}, + }, }, { name: "ItemSet", - in: "keep-sorted-test sticky_prefixes=a,b,c,d", - - want: defaultOptionsWith(func(opts *blockOptions) { - opts.StickyPrefixes = map[string]bool{"a": true, "b": true, "c": true, "d": true} - opts.commentMarker = "" - }), + in: "sticky_prefixes=a,b,c,d", + + want: blockOptions{ + StickyPrefixes: map[string]bool{ + "a": true, + "b": true, + "c": true, + "d": true, + }, + }, }, { name: "ignore_prefixes", - in: "// keep-sorted-test ignore_prefixes=a,b,c,d", + in: "ignore_prefixes=a,b,c,d", - want: defaultOptionsWith(func(opts *blockOptions) { - opts.IgnorePrefixes = []string{"a", "b", "c", "d"} - }), + want: blockOptions{ + IgnorePrefixes: []string{"a", "b", "c", "d"}, + }, }, { - name: "GroupPrefixesRequiresGrouping", - in: "// keep-sorted-test group_prefixes=a,b,c group=no", + name: "ignore_prefixes_ChecksLongestPrefixesFirst", + in: "ignore_prefixes=DoSomething(,DoSomething({", - want: defaultOptionsWith(func(opts *blockOptions) { - opts.Group = false - }), - wantErr: "group_prefixes may not be used with group=no", + want: blockOptions{ + IgnorePrefixes: []string{"DoSomething({", "DoSomething("}, + }, }, { - name: "ignore_prefixes_ChecksLognestPrefixesFirst", - in: "// keep-sorted-test ignore_prefixes=DoSomething(,DoSomething({", + name: "GroupPrefixesRequiresGrouping", + in: "group_prefixes=a,b,c group=no", - want: defaultOptionsWith(func(opts *blockOptions) { - opts.IgnorePrefixes = []string{"DoSomething({", "DoSomething("} - }), + wantErr: "group_prefixes may not be used with group=no", }, { name: "OptionInTrailingComment", - in: "// keep-sorted-test block=yes # lint=no", + in: "# keep-sorted-test start block=yes # lint=yes", - want: defaultOptionsWith(func(opts *blockOptions) { - opts.Block = true - opts.Lint = false - }), + want: blockOptions{ + Block: true, + Lint: true, + commentMarker: "#", + }, }, { name: "ErrorDoesNotStopParsing", - in: "// keep-sorted-test lint=yep case=no", + in: "lint=nah case=no", + defaultOptions: blockOptions{ + Lint: true, + CaseSensitive: true, + }, - want: defaultOptionsWith(func(opts *blockOptions) { - opts.Lint = true // The default value should not change. - opts.CaseSensitive = false - }), - wantErr: `option "lint" has unknown value "yep"`, + want: blockOptions{ + Lint: true, // The default value should not change. + CaseSensitive: false, + }, + wantErr: `option "lint" has unknown value "nah"`, }, } { t.Run(tc.name, func(t *testing.T) { initZerolog(t) - got, err := New("keep-sorted-test").parseBlockOptions(tc.in) + got, err := New("keep-sorted-test").parseBlockOptions(tc.in, tc.defaultOptions) if err != nil { if tc.wantErr == "" { t.Errorf("parseBlockOptions(%q) = %v", tc.in, err) diff --git a/keepsorted/options.go b/keepsorted/options.go index fd9ea18..1e5a72c 100644 --- a/keepsorted/options.go +++ b/keepsorted/options.go @@ -45,7 +45,7 @@ var ( // 4. int: key=123 type blockOptions struct { // Lint determines whether we emit lint warnings for this block. - Lint bool `default:"true"` + Lint bool /////////////////////////// // Pre-sorting options // @@ -54,13 +54,13 @@ type blockOptions struct { // SkipLines is the number of lines to ignore before sorting. SkipLines int `key:"skip_lines"` // Group determines whether we group lines together based on increasing indentation. - Group bool `default:"true"` + Group bool // GroupPrefixes tells us about other types of lines that should be added to a group. GroupPrefixes map[string]bool `key:"group_prefixes"` // Block opts us into a more complicated algorithm to try and understand blocks of code. - Block bool `default:"false"` + Block bool // StickyComments tells us to attach comments to the line immediately below them while sorting. - StickyComments bool `key:"sticky_comments" default:"true"` + StickyComments bool `key:"sticky_comments"` // StickyPrefixes tells us about other types of lines that should behave as sticky comments. StickyPrefixes map[string]bool `key:"sticky_prefixes"` @@ -69,9 +69,9 @@ type blockOptions struct { /////////////////////// // CaseSensitive is whether we're case sensitive while sorting. - CaseSensitive bool `key:"case" default:"true"` + CaseSensitive bool `key:"case"` // Numeric indicates that the contents should be sorted like numbers. - Numeric bool `default:"false"` + Numeric bool // PrefixOrder allows the user to explicitly order lines based on their matching prefix. PrefixOrder []string `key:"prefix_order"` // IgnorePrefixes is a slice of prefixes that we do not consider when sorting lines. @@ -82,16 +82,28 @@ type blockOptions struct { //////////////////////////// // NewlineSeparated indicates that the groups should be separated with newlines. - NewlineSeparated bool `key:"newline_separated" default:"false"` + NewlineSeparated bool `key:"newline_separated"` // RemoveDuplicates determines whether we drop lines that are an exact duplicate. - RemoveDuplicates bool `key:"remove_duplicates" default:"true"` + RemoveDuplicates bool `key:"remove_duplicates"` // Syntax used to start a comment for keep-sorted annotation, e.g. "//". commentMarker string } -func (f *Fixer) parseBlockOptions(startLine string) (blockOptions, error) { - ret := blockOptions{} +var defaultOptions = blockOptions{ + Lint: true, + Group: true, + StickyComments: true, + StickyPrefixes: nil, // Will be populated with the comment marker of the start directive. + CaseSensitive: true, + RemoveDuplicates: true, + // If we ever add a default map or slice or other reference type to this + // struct, we'll want to make sure we're doing a deep copy in + // parseBlockOptions. +} + +func (f *Fixer) parseBlockOptions(startLine string, defaults blockOptions) (blockOptions, error) { + ret := defaults opts := reflect.ValueOf(&ret) var errs error for i := 0; i < opts.Elem().NumField(); i++ { @@ -102,10 +114,13 @@ func (f *Fixer) parseBlockOptions(startLine string) (blockOptions, error) { val, err := parseBlockOption(field, startLine) if err != nil { + if errors.Is(err, errOptionNotSet) { + continue + } errs = errors.Join(errs, err) + } else { + opts.Elem().Field(i).Set(val) } - - opts.Elem().Field(i).Set(val) } if ret.SkipLines < 0 { @@ -119,13 +134,7 @@ func (f *Fixer) parseBlockOptions(startLine string) (blockOptions, error) { } if cm := f.guessCommentMarker(startLine); cm != "" { - ret.commentMarker = cm - if ret.StickyComments { - if ret.StickyPrefixes == nil { - ret.StickyPrefixes = make(map[string]bool) - } - ret.StickyPrefixes[cm] = true - } + ret.setCommentMarker(cm) } if len(ret.IgnorePrefixes) > 1 { // Look at longer prefixes first, in case one of these prefixes is a prefix of another. @@ -135,6 +144,8 @@ func (f *Fixer) parseBlockOptions(startLine string) (blockOptions, error) { return ret, errs } +var errOptionNotSet = errors.New("not set in start directive") + func parseBlockOption(f reflect.StructField, startLine string) (reflect.Value, error) { key := strings.ToLower(f.Name) if k, ok := f.Tag.Lookup("key"); ok { @@ -146,39 +157,27 @@ func parseBlockOption(f reflect.StructField, startLine string) (reflect.Value, e val := string(regex.ExpandString(nil, "${value}", startLine, m)) return parseValue(f, key, val) } - return parseDefaultValue(f, key), nil -} - -func parseDefaultValue(f reflect.StructField, key string) reflect.Value { - val, err := parseValueWithDefault(f, key, f.Tag.Get("default"), func() reflect.Value { return reflect.Zero(f.Type) }) - if err != nil { - panic(fmt.Errorf("blockOptions field %s has invalid default %q: %w", f.Name, f.Tag.Get("default"), err)) - } - return val + return reflect.Zero(f.Type), fmt.Errorf("option %q: %w", key, errOptionNotSet) } func parseValue(f reflect.StructField, key, val string) (reflect.Value, error) { - return parseValueWithDefault(f, key, val, func() reflect.Value { return parseDefaultValue(f, key) }) -} - -func parseValueWithDefault(f reflect.StructField, key, val string, defaultFn func() reflect.Value) (reflect.Value, error) { switch f.Type { case reflect.TypeOf(true): b, ok := boolValues[val] if !ok { - return defaultFn(), fmt.Errorf("option %q has unknown value %q", key, val) + return reflect.Zero(f.Type), fmt.Errorf("option %q has unknown value %q", key, val) } return reflect.ValueOf(b), nil case reflect.TypeOf([]string{}): if val == "" { - return defaultFn(), nil + return reflect.Zero(f.Type), nil } return reflect.ValueOf(strings.Split(val, ",")), nil case reflect.TypeOf(map[string]bool{}): if val == "" { - return defaultFn(), nil + return reflect.Zero(f.Type), nil } sp := strings.Split(val, ",") @@ -189,12 +188,12 @@ func parseValueWithDefault(f reflect.StructField, key, val string, defaultFn fun return reflect.ValueOf(m), nil case reflect.TypeOf(0): if val == "" { - return defaultFn(), nil + return reflect.Zero(f.Type), nil } i, err := strconv.Atoi(val) if err != nil { - return defaultFn(), fmt.Errorf("option %q has invalid value %q: %w", key, val, err) + return reflect.Zero(f.Type), fmt.Errorf("option %q has invalid value %q: %w", key, val, err) } return reflect.ValueOf(i), nil } @@ -220,6 +219,16 @@ func (f *Fixer) guessCommentMarker(startLine string) string { return "" } +func (opts *blockOptions) setCommentMarker(marker string) { + opts.commentMarker = marker + if opts.StickyComments { + if opts.StickyPrefixes == nil { + opts.StickyPrefixes = make(map[string]bool) + } + opts.StickyPrefixes[marker] = true + } +} + // hasPrefix determines if s has one of the prefixes. func hasPrefix(s string, prefixes map[string]bool) bool { if len(prefixes) == 0 { From a6de5739716b3d9458b61d3f304268890aea6035 Mon Sep 17 00:00:00 2001 From: Jeffrey Faer Date: Mon, 8 Apr 2024 08:12:43 -0600 Subject: [PATCH 2/4] feat: Allow default options to be customzied with a flag. --- cmd/cmd.go | 43 ++++++++-- keepsorted/block.go | 4 +- keepsorted/keep_sorted.go | 4 +- keepsorted/keep_sorted_test.go | 150 +++++++++++++++++++++++---------- keepsorted/options.go | 89 ++++++++++++++++--- 5 files changed, 222 insertions(+), 68 deletions(-) diff --git a/cmd/cmd.go b/cmd/cmd.go index 949bf52..e9fa308 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -30,9 +30,10 @@ import ( ) type Config struct { - id string - operation operation - modifiedLines []keepsorted.LineRange + id string + defaultOptions keepsorted.BlockOptions + operation operation + modifiedLines []keepsorted.LineRange } func (c *Config) FromFlags(fs *flag.FlagSet) { @@ -45,6 +46,9 @@ func (c *Config) FromFlags(fs *flag.FlagSet) { panic(err) } + c.defaultOptions = keepsorted.DefaultBlockOptions() + fs.Var(&blockOptionsFlag{&c.defaultOptions}, "default-options", "The options keep-sorted will use to sort. Per-block overrides apply on top of these options. Note: list options like prefix_order are not merged with per-block overrides. They are completely overridden.") + of := &operationFlag{op: &c.operation} if err := of.Set("fix"); err != nil { panic(err) @@ -54,6 +58,27 @@ func (c *Config) FromFlags(fs *flag.FlagSet) { fs.Var(&lineRangeFlag{lineRanges: &c.modifiedLines}, "lines", "Line ranges of the form \"start:end\". Only processes keep-sorted blocks that overlap with the given line ranges. Can only be used when fixing a single file.") } +type blockOptionsFlag struct { + opts *keepsorted.BlockOptions +} + +func (f *blockOptionsFlag) String() string { + return f.opts.String() +} + +func (f *blockOptionsFlag) Set(val string) error { + opts, err := keepsorted.ParseBlockOptions(val) + if err != nil { + return err + } + *f.opts = opts + return nil +} + +func (f *blockOptionsFlag) Type() string { + return "options" +} + var ( operations = map[string]operation{ "lint": lint, @@ -67,7 +92,7 @@ func knownModes() []string { return ms } -type operation func(id string, filenames []string, modifiedLines []keepsorted.LineRange) (ok bool, err error) +type operation func(fixer *keepsorted.Fixer, filenames []string, modifiedLines []keepsorted.LineRange) (ok bool, err error) type operationFlag struct { op *operation @@ -185,16 +210,16 @@ func Run(c *Config, files []string) (ok bool, err error) { return false, errors.New("cannot specify modifiedLines with more than one file") } - return c.operation(c.id, files, c.modifiedLines) + return c.operation(keepsorted.New(c.id, c.defaultOptions), files, c.modifiedLines) } -func fix(id string, filenames []string, modifiedLines []keepsorted.LineRange) (ok bool, err error) { +func fix(fixer *keepsorted.Fixer, filenames []string, modifiedLines []keepsorted.LineRange) (ok bool, err error) { for _, fn := range filenames { contents, err := read(fn) if err != nil { return false, err } - if want, alreadyFixed := keepsorted.New(id).Fix(contents, modifiedLines); fn == stdin || !alreadyFixed { + if want, alreadyFixed := fixer.Fix(contents, modifiedLines); fn == stdin || !alreadyFixed { if err := write(fn, want); err != nil { return false, err } @@ -203,14 +228,14 @@ func fix(id string, filenames []string, modifiedLines []keepsorted.LineRange) (o return true, nil } -func lint(id string, filenames []string, modifiedLines []keepsorted.LineRange) (ok bool, err error) { +func lint(fixer *keepsorted.Fixer, filenames []string, modifiedLines []keepsorted.LineRange) (ok bool, err error) { var fs []*keepsorted.Finding for _, fn := range filenames { contents, err := read(fn) if err != nil { return false, err } - fs = append(fs, keepsorted.New(id).Findings(fn, contents, modifiedLines)...) + fs = append(fs, fixer.Findings(fn, contents, modifiedLines)...) } if len(fs) == 0 { diff --git a/keepsorted/block.go b/keepsorted/block.go index c292c85..b094241 100644 --- a/keepsorted/block.go +++ b/keepsorted/block.go @@ -109,7 +109,7 @@ func (f *Fixer) newBlocks(lines []string, offset int, include func(start, end in continue } - opts, err := f.parseBlockOptions(start.line, defaultOptions) + opts, err := parseBlockOptions(start.line, f.defaultOptions) if err != nil { // TODO(b/250608236): Is there a better way to surface this error? log.Err(fmt.Errorf("keep-sorted block at index %d had bad start directive: %w", start.index+offset, err)).Msg("") @@ -238,7 +238,7 @@ func (b block) sorted() (sorted []string, alreadySorted bool) { } groups := groupLines(lines, b.metadata) - log.Printf("%d groups for block at index %d are (options %#v)", len(groups), b.start, b.metadata.opts) + log.Printf("%d groups for block at index %d are (options %v)", len(groups), b.start, b.metadata.opts) for _, lg := range groups { log.Printf("%#v", lg) } diff --git a/keepsorted/keep_sorted.go b/keepsorted/keep_sorted.go index 4196d58..45b9a03 100644 --- a/keepsorted/keep_sorted.go +++ b/keepsorted/keep_sorted.go @@ -31,15 +31,17 @@ const ( type Fixer struct { ID string + defaultOptions blockOptions startDirective string endDirective string } // New creates a new fixer with the given string as its identifier. // By default, id is "keep-sorted" -func New(id string) *Fixer { +func New(id string, defaultOptions BlockOptions) *Fixer { return &Fixer{ ID: id, + defaultOptions: defaultOptions.opts, startDirective: id + " start", endDirective: id + " end", } diff --git a/keepsorted/keep_sorted_test.go b/keepsorted/keep_sorted_test.go index 307030f..e3be0bf 100644 --- a/keepsorted/keep_sorted_test.go +++ b/keepsorted/keep_sorted_test.go @@ -15,10 +15,13 @@ package keepsorted import ( + "fmt" + "reflect" "strings" "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/rs/zerolog" "github.com/rs/zerolog/log" ) @@ -31,20 +34,6 @@ func initZerolog(t testing.TB) { t.Cleanup(func() { log.Logger = oldLogger }) } -func defaultOptionsWithCommentMarker(marker string) blockOptions { - opts := defaultOptions - opts.StickyComments = true - opts.setCommentMarker(marker) - return opts -} - -func optionsWithCommentMarker(marker string) blockOptions { - var opts blockOptions - opts.StickyComments = true - opts.setCommentMarker(marker) - return opts -} - func defaultMetadataWith(opts blockOptions) blockMetadata { return blockMetadata{ startDirective: "keep-sorted-test start", @@ -53,6 +42,12 @@ func defaultMetadataWith(opts blockOptions) blockMetadata { } } +func defaultMetadataWithCommentMarker(marker string) blockMetadata { + var opts blockOptions + opts.setCommentMarker(marker) + return defaultMetadataWith(opts) +} + func TestFix(t *testing.T) { for _, tc := range []struct { name string @@ -179,7 +174,7 @@ foo } { t.Run(tc.name, func(t *testing.T) { initZerolog(t) - got, gotAlreadyFixed := New("keep-sorted-test").Fix(tc.in, nil) + got, gotAlreadyFixed := New("keep-sorted-test", BlockOptions{}).Fix(tc.in, nil) if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("Fix diff (-want +got):\n%s", diff) } @@ -320,7 +315,7 @@ baz mod = append(mod, LineRange{l, l}) } } - got := New("keep-sorted-test").findings(filename, strings.Split(tc.in, "\n"), mod, tc.considerLintOption) + got := New("keep-sorted-test", BlockOptions{}).findings(filename, strings.Split(tc.in, "\n"), mod, tc.considerLintOption) if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("Findings diff (-want +got):\n%s", diff) } @@ -360,7 +355,7 @@ cat`, wantBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 3, end: 7, lines: []string{ @@ -370,7 +365,7 @@ cat`, }, }, { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 9, end: 13, lines: []string{ @@ -397,7 +392,7 @@ dog wantBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 5, end: 7, lines: []string{ @@ -435,7 +430,7 @@ cat`, wantBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 3, end: 7, lines: []string{ @@ -463,7 +458,7 @@ cat`, wantBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 1, end: 6, lines: []string{ @@ -496,7 +491,7 @@ i wantBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 1, end: 13, lines: []string{ @@ -514,7 +509,7 @@ i }, nestedBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 5, end: 9, lines: []string{ @@ -574,7 +569,7 @@ i wantBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 1, end: 34, lines: []string{ @@ -613,7 +608,7 @@ i }, nestedBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 5, end: 30, lines: []string{ @@ -644,7 +639,7 @@ i }, nestedBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 9, end: 21, lines: []string{ @@ -662,7 +657,7 @@ i }, nestedBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 13, end: 17, lines: []string{ @@ -674,7 +669,7 @@ i }, }, { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 22, end: 26, lines: []string{ @@ -688,7 +683,7 @@ i }, }, { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 35, end: 39, lines: []string{ @@ -714,7 +709,7 @@ i wantBlocks: []block{ { - metadata: defaultMetadataWith(defaultOptionsWithCommentMarker("//")), + metadata: defaultMetadataWithCommentMarker("//"), start: 5, end: 7, lines: []string{"2"}, @@ -732,7 +727,7 @@ i tc.include = func(start, end int) bool { return true } } - gotBlocks, gotIncompleteBlocks := New("keep-sorted-test").newBlocks(strings.Split(tc.in, "\n"), 0, tc.include) + gotBlocks, gotIncompleteBlocks := New("keep-sorted-test", BlockOptions{}).newBlocks(strings.Split(tc.in, "\n"), 0, tc.include) if diff := cmp.Diff(tc.wantBlocks, gotBlocks, cmp.AllowUnexported(block{}, blockMetadata{}, blockOptions{})); diff != "" { t.Errorf("blocks diff (-want +got):\n%s", diff) } @@ -861,7 +856,13 @@ func TestLineSorting(t *testing.T) { { name: "CommentOnlyBlock", - opts: optionsWithCommentMarker("//"), + opts: func() blockOptions { + opts := blockOptions{ + StickyComments: true, + } + opts.setCommentMarker("//") + return opts + }(), in: []string{ "2", "1", @@ -919,8 +920,11 @@ func TestLineSorting(t *testing.T) { name: "RemoveDuplicates_ConsidersComments", opts: func() blockOptions { - opts := optionsWithCommentMarker("//") - opts.RemoveDuplicates = true + opts := blockOptions{ + RemoveDuplicates: true, + StickyComments: true, + } + opts.setCommentMarker("//") return opts }(), in: []string{ @@ -1180,7 +1184,13 @@ func TestLineGrouping(t *testing.T) { }, { name: "StickyComments", - opts: optionsWithCommentMarker("//"), + opts: func() blockOptions { + opts := blockOptions{ + StickyComments: true, + } + opts.setCommentMarker("//") + return opts + }(), want: []lineGroup{ { @@ -1203,7 +1213,13 @@ func TestLineGrouping(t *testing.T) { }, { name: "CommentOnlyGroup", - opts: optionsWithCommentMarker("//"), + opts: func() blockOptions { + opts := blockOptions{ + StickyComments: true, + } + opts.setCommentMarker("//") + return opts + }(), want: []lineGroup{ { @@ -1291,8 +1307,11 @@ func TestLineGrouping(t *testing.T) { { name: "Group_NestedKeepSortedBlocksWithoutAnyIndentation", opts: func() blockOptions { - opts := optionsWithCommentMarker("//") - opts.Group = true + opts := blockOptions{ + Group: true, + StickyComments: true, + } + opts.setCommentMarker("//") return opts }(), @@ -1425,8 +1444,10 @@ func TestLineGrouping(t *testing.T) { { name: "Block_IgnoresSpecialCharactersWithinFullLineComments", opts: func() blockOptions { - opts := optionsWithCommentMarker("//") - opts.Block = true + opts := blockOptions{ + Block: true, + } + opts.setCommentMarker("//") return opts }(), @@ -1450,8 +1471,10 @@ func TestLineGrouping(t *testing.T) { { name: "Block_IgnoresSpecialCharactersWithinTrailingComments", opts: func() blockOptions { - opts := optionsWithCommentMarker("//") - opts.Block = true + opts := blockOptions{ + Block: true, + } + opts.setCommentMarker("//") return opts }(), @@ -1628,7 +1651,7 @@ func TestBlockOptions(t *testing.T) { } { t.Run(tc.name, func(t *testing.T) { initZerolog(t) - got, err := New("keep-sorted-test").parseBlockOptions(tc.in, tc.defaultOptions) + got, err := parseBlockOptions(tc.in, tc.defaultOptions) if err != nil { if tc.wantErr == "" { t.Errorf("parseBlockOptions(%q) = %v", tc.in, err) @@ -1639,6 +1662,47 @@ func TestBlockOptions(t *testing.T) { if diff := cmp.Diff(tc.want, got, cmp.AllowUnexported(blockOptions{})); diff != "" { t.Errorf("parseBlockOptions(%q) mismatch (-want +got):\n%s", tc.in, diff) } + + _ = got.String() // Make sure this doesn't panic. }) } } + +func TestBlockOptions_ClonesDefaultOptions(t *testing.T) { + defaults := blockOptions{ + StickyPrefixes: map[string]bool{}, + } + _, err := parseBlockOptions("sticky_prefixes=//", defaults) + if err != nil { + t.Errorf("parseBlockOptions() = _, %v", err) + } + if diff := cmp.Diff(blockOptions{}, defaults, cmp.AllowUnexported(blockOptions{}), cmpopts.EquateEmpty()); diff != "" { + t.Errorf("defaults appear to have been modified (-want +got):\n%s", diff) + } +} + +func TestBlockOptions_ClonesDefaultOptions_Reflection(t *testing.T) { + defaults := blockOptions{} + defaultOpts := reflect.ValueOf(&defaults).Elem() + var s []string + for i := 0; i < defaultOpts.NumField(); i++ { + val := defaultOpts.Field(i) + switch val.Kind() { + case reflect.Bool, reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Float32, reflect.Float64, reflect.Complex64, reflect.Complex128, reflect.String: + continue + case reflect.Slice: + val.Set(reflect.MakeSlice(val.Type(), 0, 0)) + s = append(s, fmt.Sprintf("%s=a,b,c", key(defaultOpts.Type().Field(i)))) + case reflect.Map: + val.Set(reflect.MakeMap(val.Type())) + s = append(s, fmt.Sprintf("%s=a,b,c", key(defaultOpts.Type().Field(i)))) + default: + t.Fatalf("unhandled type in blockOptions: %v", val.Type()) + } + + } + _, _ = parseBlockOptions(strings.Join(s, " "), defaults) + if diff := cmp.Diff(blockOptions{}, defaults, cmp.AllowUnexported(blockOptions{}), cmpopts.EquateEmpty()); diff != "" { + t.Errorf("defaults appear to have been modified (-want +got):\n%s", diff) + } +} diff --git a/keepsorted/options.go b/keepsorted/options.go index 1e5a72c..a2ce6f0 100644 --- a/keepsorted/options.go +++ b/keepsorted/options.go @@ -25,6 +25,8 @@ import ( "strconv" "strings" "unicode" + + "golang.org/x/exp/maps" ) var ( @@ -34,8 +36,32 @@ var ( "no": false, "false": false, } + boolString = map[bool]string{ + true: "yes", + false: "no", + } ) +type BlockOptions struct { + opts blockOptions +} + +func DefaultBlockOptions() BlockOptions { + return BlockOptions{defaultOptions} +} + +func ParseBlockOptions(startLine string) (BlockOptions, error) { + opts, err := parseBlockOptions(startLine, blockOptions{}) + if err != nil { + return BlockOptions{}, err + } + return BlockOptions{opts}, nil +} + +func (opts BlockOptions) String() string { + return opts.opts.String() +} + // blockOptions enable/disable extra features that control how a block of lines is sorted. // // Currently, only four types are supported: @@ -97,12 +123,9 @@ var defaultOptions = blockOptions{ StickyPrefixes: nil, // Will be populated with the comment marker of the start directive. CaseSensitive: true, RemoveDuplicates: true, - // If we ever add a default map or slice or other reference type to this - // struct, we'll want to make sure we're doing a deep copy in - // parseBlockOptions. } -func (f *Fixer) parseBlockOptions(startLine string, defaults blockOptions) (blockOptions, error) { +func parseBlockOptions(startLine string, defaults blockOptions) (blockOptions, error) { ret := defaults opts := reflect.ValueOf(&ret) var errs error @@ -133,7 +156,7 @@ func (f *Fixer) parseBlockOptions(startLine string, defaults blockOptions) (bloc ret.GroupPrefixes = nil } - if cm := f.guessCommentMarker(startLine); cm != "" { + if cm := guessCommentMarker(startLine); cm != "" { ret.setCommentMarker(cm) } if len(ret.IgnorePrefixes) > 1 { @@ -147,11 +170,7 @@ func (f *Fixer) parseBlockOptions(startLine string, defaults blockOptions) (bloc var errOptionNotSet = errors.New("not set in start directive") func parseBlockOption(f reflect.StructField, startLine string) (reflect.Value, error) { - key := strings.ToLower(f.Name) - if k, ok := f.Tag.Lookup("key"); ok { - key = k - } - + key := key(f) regex := regexp.MustCompile(fmt.Sprintf(`(^|\s)%s=(?P[^ ]+?)($|\s)`, regexp.QuoteMeta(key))) if m := regex.FindStringSubmatchIndex(startLine); m != nil { val := string(regex.ExpandString(nil, "${value}", startLine, m)) @@ -160,9 +179,17 @@ func parseBlockOption(f reflect.StructField, startLine string) (reflect.Value, e return reflect.Zero(f.Type), fmt.Errorf("option %q: %w", key, errOptionNotSet) } +func key(f reflect.StructField) string { + key := strings.ToLower(f.Name) + if k, ok := f.Tag.Lookup("key"); ok { + key = k + } + return key +} + func parseValue(f reflect.StructField, key, val string) (reflect.Value, error) { switch f.Type { - case reflect.TypeOf(true): + case reflect.TypeOf(bool(false)): b, ok := boolValues[val] if !ok { return reflect.Zero(f.Type), fmt.Errorf("option %q has unknown value %q", key, val) @@ -186,7 +213,7 @@ func parseValue(f reflect.StructField, key, val string) (reflect.Value, error) { m[s] = true } return reflect.ValueOf(m), nil - case reflect.TypeOf(0): + case reflect.TypeOf(int(0)): if val == "" { return reflect.Zero(f.Type), nil } @@ -201,7 +228,24 @@ func parseValue(f reflect.StructField, key, val string) (reflect.Value, error) { panic(fmt.Errorf("unsupported blockOptions type: %v", f.Type)) } -func (f *Fixer) guessCommentMarker(startLine string) string { +func formatValue(val reflect.Value) string { + switch val.Type() { + case reflect.TypeOf(bool(false)): + return boolString[val.Bool()] + case reflect.TypeOf([]string{}): + return strings.Join(val.Interface().([]string), ",") + case reflect.TypeOf(map[string]bool{}): + keys := maps.Keys(val.Interface().(map[string]bool)) + slices.Sort(keys) + return strings.Join(keys, ",") + case reflect.TypeOf(int(0)): + return strconv.Itoa(int(val.Int())) + } + + panic(fmt.Errorf("unsupported blockOptions type: %v", val.Type())) +} + +func guessCommentMarker(startLine string) string { startLine = strings.TrimSpace(startLine) if strings.HasPrefix(startLine, "//") { return "//" @@ -229,6 +273,25 @@ func (opts *blockOptions) setCommentMarker(marker string) { } } +func (opts blockOptions) String() string { + var s []string + val := reflect.ValueOf(opts) + for i := 0; i < val.NumField(); i++ { + field := val.Type().Field(i) + if !field.IsExported() { + continue + } + + fieldVal := val.FieldByIndex(field.Index) + if fieldVal.IsZero() { + continue + } + s = append(s, fmt.Sprintf("%s=%s", key(field), formatValue(fieldVal))) + } + + return strings.Join(s, " ") +} + // hasPrefix determines if s has one of the prefixes. func hasPrefix(s string, prefixes map[string]bool) bool { if len(prefixes) == 0 { From 023bc0853ca7685294dff7fbe778766095593c29 Mon Sep 17 00:00:00 2001 From: Jeffrey Faer Date: Tue, 9 Apr 2024 10:46:07 -0600 Subject: [PATCH 3/4] fix some flag --help text. --- cmd/cmd.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cmd/cmd.go b/cmd/cmd.go index e9fa308..75fa451 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -55,7 +55,7 @@ func (c *Config) FromFlags(fs *flag.FlagSet) { } fs.Var(of, "mode", fmt.Sprintf("Determines what mode to run this tool in. One of %q", knownModes())) - fs.Var(&lineRangeFlag{lineRanges: &c.modifiedLines}, "lines", "Line ranges of the form \"start:end\". Only processes keep-sorted blocks that overlap with the given line ranges. Can only be used when fixing a single file.") + fs.Var(&lineRangeFlag{lineRanges: &c.modifiedLines}, "lines", "Line ranges of the form \"start:end\". Only processes keep-sorted blocks that overlap with the given line ranges. Can only be used when fixing a single file. This flag can either be a comma-separated list of line ranges, or it can be specified multiple times on the command line to specify multiple line ranges.") } type blockOptionsFlag struct { @@ -108,6 +108,7 @@ func (f *operationFlag) Set(val string) error { if op == nil { return fmt.Errorf("unknown mode %q. Valid modes: %q", val, knownModes()) } + f.s = val *f.op = op return nil } @@ -141,7 +142,7 @@ func (f *lineRangeFlag) Set(val string) error { } func (f *lineRangeFlag) Type() string { - return "line ranges" + return "line_ranges" } func (f *lineRangeFlag) Append(val string) error { From 08e1cb1f6887a1d8ccd62702d085bec5ffc27165 Mon Sep 17 00:00:00 2001 From: Jeffrey Faer Date: Tue, 9 Apr 2024 13:42:32 -0600 Subject: [PATCH 4/4] address review feedback --- keepsorted/keep_sorted_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/keepsorted/keep_sorted_test.go b/keepsorted/keep_sorted_test.go index e3be0bf..0abcd04 100644 --- a/keepsorted/keep_sorted_test.go +++ b/keepsorted/keep_sorted_test.go @@ -1697,7 +1697,7 @@ func TestBlockOptions_ClonesDefaultOptions_Reflection(t *testing.T) { val.Set(reflect.MakeMap(val.Type())) s = append(s, fmt.Sprintf("%s=a,b,c", key(defaultOpts.Type().Field(i)))) default: - t.Fatalf("unhandled type in blockOptions: %v", val.Type()) + t.Errorf("Option %q has unhandled type: %v", key(defaultOpts.Type().Field(i)), val.Type()) } }