Fix mismatching marker handling, add better error handling (#51)

* Add errorsplus for composite errors

* Adjust error message

* Change error message to human friendly message

* Remove duplicated message

* Add indentation keep support, and test case for align

* Add proper error handling when markers don't match

* Adjust error message

* Add clarification comment

* Add better error handling test case

* Add test case for indent keep mode

* Add test case for broken marker

* Correct error message

Author: rytswd

internal/cli/generate.go

@@ -44,7 +44,7 @@ func executeGenerate(cmd *cobra.Command, args []string) error {
 	arg := args[0]
 	out := generateTargetFile
 	if err := generate(arg, out); err != nil {
-		return fmt.Errorf("handling generate, %v", err)
+		return fmt.Errorf("failed to generate for '%s', %v", arg, err)
 	}
 
 	return nil

internal/cli/purge.go

@@ -7,6 +7,7 @@ import (
 
 	"github.com/spf13/cobra"
 
+	"github.com/upsidr/importer/internal/errorsplus"
 	"github.com/upsidr/importer/internal/file"
 	"github.com/upsidr/importer/internal/parse"
 )
@@ -38,11 +39,15 @@ func executePurge(cmd *cobra.Command, args []string) error {
 	// Suppress usage message after this point
 	cmd.SilenceUsage = true
 
+	errs := errorsplus.Errors{}
 	for _, file := range args {
 		if err := purge(file); err != nil {
-			fmt.Printf("Warning: failed to purge for '%s', %v", file, err)
+			errs = append(errs, fmt.Errorf("failed to update '%s', %v", file, err))
 		}
 	}
+	if len(errs) != 0 {
+		return errs
+	}
 
 	return nil
 }

internal/cli/update.go

@@ -7,6 +7,7 @@ import (
 
 	"github.com/spf13/cobra"
 
+	"github.com/upsidr/importer/internal/errorsplus"
 	"github.com/upsidr/importer/internal/file"
 	"github.com/upsidr/importer/internal/parse"
 )
@@ -39,11 +40,15 @@ func executeUpdate(cmd *cobra.Command, args []string) error {
 	// Suppress usage message after this point
 	cmd.SilenceUsage = true
 
+	errs := errorsplus.Errors{}
 	for _, file := range args {
 		if err := update(file); err != nil {
-			fmt.Printf("Warning: failed to generate for '%s', %v", file, err)
+			errs = append(errs, fmt.Errorf("failed to update '%s', %v", file, err))
 		}
 	}
+	if len(errs) != 0 {
+		return errs
+	}
 
 	return nil
 }

internal/errorsplus/composite_errors.go

@@ -0,0 +1,39 @@
+package errorsplus
+
+import (
+	"errors"
+	"fmt"
+)
+
+// Errors represents a slice of errors.
+type Errors []error
+
+func (es Errors) Error() string {
+	switch len(es) {
+	case 0:
+		return ""
+	case 1:
+		return fmt.Sprintf("%v", es[0])
+	}
+
+	rt := "more than one error occurred:"
+	for _, e := range es {
+		if e != nil {
+			rt = fmt.Sprintf("%s\n\t%v", rt, e)
+		}
+	}
+	return rt
+}
+
+func (es Errors) Is(target error) bool {
+	if len(es) == 0 {
+		return false
+	}
+
+	for _, e := range es {
+		if errors.Is(e, target) {
+			return true
+		}
+	}
+	return false
+}

internal/errorsplus/composite_errors_test.go

@@ -0,0 +1,122 @@
+package errorsplus_test
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+
+	"github.com/upsidr/importer/internal/errorsplus"
+)
+
+var (
+	errFirst  = errors.New("first error")
+	errSecond = errors.New("second error")
+	errThird  = errors.New("third error")
+	errOther  = errors.New("some other error")
+)
+
+func TestCompositeErrors(t *testing.T) {
+	cases := map[string]struct {
+		errs          errorsplus.Errors
+		wantErr       error
+		wantErrString string
+	}{
+		"zero error": {
+			errs: errorsplus.Errors{
+				// empty
+			},
+			wantErr:       nil, // cannot be checked against nil, this is skipped explicitly below
+			wantErrString: "",  // empty
+		},
+		"single error": {
+			errs: errorsplus.Errors{
+				errFirst,
+			},
+			wantErr:       errFirst,
+			wantErrString: "first error",
+		},
+		"2 errors": {
+			errs: errorsplus.Errors{
+				errFirst,
+				errSecond,
+			},
+			wantErr: errFirst,
+			wantErrString: `more than one error occurred:
+	first error
+	second error`,
+		},
+		"3 errors": {
+			errs: errorsplus.Errors{
+				errFirst,
+				errSecond,
+				errThird,
+			},
+			wantErr: errFirst,
+			wantErrString: `more than one error occurred:
+	first error
+	second error
+	third error`,
+		},
+		"duplicated errors": {
+			errs: errorsplus.Errors{
+				errFirst,
+				errFirst,
+				errFirst,
+			},
+			wantErr: errFirst,
+			wantErrString: `more than one error occurred:
+	first error
+	first error
+	first error`,
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			if tc.wantErr != nil && !errors.Is(tc.errs, tc.wantErr) {
+				t.Errorf("error mismatch\n\tcomposite error: %v\n\twanted error: %v", tc.errs, tc.wantErr)
+			}
+
+			errString := tc.errs.Error()
+			if diff := cmp.Diff(tc.wantErrString, errString); diff != "" {
+				t.Errorf("result didn't match (-want / +got)\n%s", diff)
+			}
+		})
+	}
+}
+
+func TestCompositeErrorsMismatch(t *testing.T) {
+	cases := map[string]struct {
+		errs          errorsplus.Errors
+		mismatchedErr error
+	}{
+		"zero error": {
+			errs: errorsplus.Errors{
+				// empty
+			},
+			mismatchedErr: errOther,
+		},
+		"single error": {
+			errs: errorsplus.Errors{
+				errFirst,
+			},
+			mismatchedErr: errOther,
+		},
+		"2 errors": {
+			errs: errorsplus.Errors{
+				errFirst,
+				errSecond,
+			},
+			mismatchedErr: errOther,
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			if errors.Is(tc.errs, tc.mismatchedErr) {
+				t.Errorf("unexpected error match, where it should not be matched with errors.Is\n\tcomposite error: %v\n\ttarget error: %v", tc.errs, tc.mismatchedErr)
+			}
+		})
+	}
+}

internal/marker/errors.go

@@ -7,7 +7,7 @@ var (
 	ErrNoInput             = errors.New("no file content found")
 	ErrInvalidPath         = errors.New("invalid path provided")
 	ErrInvalidSyntax       = errors.New("invalid syntax given")
-	ErrNoMatchingMarker    = errors.New("no matching marker found, marker must be a begin/end pair")
+	ErrNoMatchingMarker    = errors.New("no matching marker found")
 	ErrMissingOption       = errors.New("missing option")
 	ErrMissingName         = errors.New("unnamed marker cannot be used")
 	ErrNoFileInput         = errors.New("no target file input provided")

internal/marker/marker.go

@@ -80,7 +80,7 @@ func NewMarker(raw *RawMarker) (*Marker, error) {
 func processFileOption(marker *Marker, match *RawMarker) error {
 	matches, err := regexpplus.MapWithNamedSubgroups(match.Options, OptionFilePathIndicator)
 	if err != nil {
-		return fmt.Errorf("%w, import target option is missing", ErrInvalidSyntax)
+		return fmt.Errorf("%w for '%s', import target option is missing", ErrInvalidSyntax, match.Name)
 	}
 
 	if targetPath, found := matches["importer_target_path"]; found {
@@ -116,6 +116,12 @@ func processIndentOption(marker *Marker, match *RawMarker) error {
 				MarkerIndentation: markerIndentation,
 			}
 			return nil // Align option does not care length information
+		case "keep":
+			// Keep the provided indentation, and do nothing
+			marker.Indentation = &Indentation{
+				Mode: KeepIndentation,
+			}
+			return nil
 		default:
 			return errors.New("unsupported indentation mode") // This shouldn't happen with the underlying regex
 		}
@@ -128,7 +134,7 @@ func processIndentOption(marker *Marker, match *RawMarker) error {
 
 		length, err := strconv.Atoi(lengthInput)
 		if err != nil {
-			return fmt.Errorf("%w, %v", ErrInvalidSyntax, err)
+			return fmt.Errorf("%w for '%s', %v", ErrInvalidSyntax, marker.Name, err)
 		}
 		marker.Indentation.Length = length
 	}
@@ -217,15 +223,15 @@ func processTargetDetail(marker *Marker, input string) error {
 	case strings.Contains(input, "~"):
 		lb, ub, err := getLineRangeWithTilde(input)
 		if err != nil {
-			return fmt.Errorf("%w, %v", ErrInvalidSyntax, err)
+			return fmt.Errorf("%w for '%s', %v", ErrInvalidSyntax, marker.Name, err)
 		}
 		marker.TargetLineFrom = lb
 		marker.TargetLineTo = ub
 
 	default:
 		i, err := strconv.Atoi(input)
 		if err != nil {
-			return fmt.Errorf("%w, %v", ErrInvalidSyntax, err)
+			return fmt.Errorf("%w for '%s', %v", ErrInvalidSyntax, marker.Name, err)
 		}
 		marker.TargetLines = append(marker.TargetLines, i)
 	}
@@ -234,8 +240,9 @@ func processTargetDetail(marker *Marker, input string) error {
 }
 
 var (
-	errLowerBound = errors.New("invalid lower bound in line range")
-	errUpperBound = errors.New("invalid upper bound in line range")
+	errLowerBound     = errors.New("invalid lower bound in line range")
+	errUpperBound     = errors.New("invalid upper bound in line range")
+	errMultipleTildes = errors.New("tilde cannot be used more than once")
 )
 
 func getLineRangeWithTilde(input string) (int, int, error) {
@@ -244,7 +251,7 @@ func getLineRangeWithTilde(input string) (int, int, error) {
 
 	ls := strings.Split(input, "~")
 	if len(ls) > 2 {
-		return lowerBound, upperBound, fmt.Errorf("%w, tilde cannot be used more than once", ErrInvalidSyntax)
+		return lowerBound, upperBound, fmt.Errorf("%w", errMultipleTildes)
 	}
 
 	lb := ls[0]

internal/marker/marker_regex.go

@@ -15,7 +15,7 @@ var (
 	OptionFilePathIndicator = `from: (?P<importer_target_path>\S+)\s*\#(?P<importer_target_detail>[0-9a-zA-Z,-_\~]+)\s?`
 
 	// OptionIndentMode is the pattern used for specifying indentation mode.
-	OptionIndentMode = `indent: (?P<importer_indent_mode>absolute|extra|align)\s?(?P<importer_indent_length>\d*)`
+	OptionIndentMode = `indent: (?P<importer_indent_mode>absolute|extra|align|keep)\s?(?P<importer_indent_length>\d*)`
 )
 
 var (

internal/marker/marker_test.go

@@ -120,6 +120,45 @@ func TestNewMarker(t *testing.T) {
 				},
 			},
 		},
+		"Exporter with indent align": {
+			input: &marker.RawMarker{
+				Name:                 "simple-marker",
+				IsBeginFound:         true,
+				IsEndFound:           true,
+				LineToInsertAt:       3,
+				Options:              "from: ./abc.yaml#[from-exporter-marker] indent: align",
+				PrecedingIndentation: "    ",
+			},
+			want: &marker.Marker{
+				Name:               "simple-marker",
+				LineToInsertAt:     3,
+				TargetPath:         "./abc.yaml",
+				TargetExportMarker: "from-exporter-marker",
+				Indentation: &marker.Indentation{
+					Mode:              marker.AlignIndentation,
+					MarkerIndentation: 4,
+				},
+			},
+		},
+		"Exporter with indent keep": {
+			input: &marker.RawMarker{
+				Name:                 "simple-marker",
+				IsBeginFound:         true,
+				IsEndFound:           true,
+				LineToInsertAt:       3,
+				Options:              "from: ./abc.yaml#[from-exporter-marker] indent: keep",
+				PrecedingIndentation: "    ",
+			},
+			want: &marker.Marker{
+				Name:               "simple-marker",
+				LineToInsertAt:     3,
+				TargetPath:         "./abc.yaml",
+				TargetExportMarker: "from-exporter-marker",
+				Indentation: &marker.Indentation{
+					Mode: marker.KeepIndentation,
+				},
+			},
+		},
 	}
 
 	for name, tc := range cases {

internal/marker/raw_marker.go

@@ -17,11 +17,11 @@ type RawMarker struct {
 // Validate checks RawMarker's validity.
 func (r *RawMarker) Validate() error {
 	if r.Name == "" {
-		return fmt.Errorf("%w", ErrMissingName)
+		return fmt.Errorf("%w", ErrMissingName) // Should not happen with the current regexp setup
 	}
 
 	if !r.IsBeginFound || !r.IsEndFound {
-		return fmt.Errorf("%w", ErrNoMatchingMarker)
+		return fmt.Errorf("%w for '%s', marker must be a begin/end pair", ErrNoMatchingMarker, r.Name)
 	}
 
 	return nil