Skip to content
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.

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

Support error types #240

Merged
merged 13 commits into from Feb 7, 2023
Merged

Support error types #240

merged 13 commits into from Feb 7, 2023

Conversation

akutuev
Copy link
Contributor

@akutuev akutuev commented Dec 7, 2022

This is an enhancement related to that discussion: #238

The purpose of this PR is to make a possibility convert an error to its specific type.

Since the errors aggregation has been added before (#233), user should convert an error result to AggregateError first and then iterate over accumulated errors, example:

        err := Parse(&config{})
	if e, ok := err.(*AggregateError); ok {
		for _, er := range e.Errors {
			switch v := er.(type) {
			case ParseError:
				// handle it
			case NotStructPtrError:
				// handle it
			case NoParserError:
				// handle it
			case NoSupportedTagOptionError:
				// handle it
			default:
				fmt.Printf("Unknown type %v", v)
			}
		}
	}

@codecov
Copy link

codecov bot commented Dec 7, 2022

Codecov Report

Base: 100.00% // Head: 99.74% // Decreases project coverage by -0.25% ⚠️

Coverage data is based on head (d9976c3) compared to base (da848aa).
Patch coverage: 98.41% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##              main     #240      +/-   ##
===========================================
- Coverage   100.00%   99.74%   -0.26%     
===========================================
  Files            2        3       +1     
  Lines          356      386      +30     
===========================================
+ Hits           356      385      +29     
- Misses           0        1       +1     
Impacted Files Coverage Δ
env.go 99.70% <94.44%> (-0.30%) ⬇️
error.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@akutuev
Copy link
Contributor Author

akutuev commented Dec 8, 2022

@caarlos0 @jurajkulich please feel free to review and validate it

Any comments appreciated

Thanks

Copy link
Owner

@caarlos0 caarlos0 left a comment

Choose a reason for hiding this comment

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

a few comments, but looking good so far!
thanks for the PR :D

error.go Outdated
Comment on lines 55 to 66
// ErrNotAStructPtr is returned if you pass something that is not a pointer to a Struct to Parse.
type NotStructPtrError struct {
msg string
}

func newNotStructPtrError() error {
return NotStructPtrError{"expected a pointer to a Struct"}
}

func (e NotStructPtrError) Error() string {
return e.msg
}
Copy link
Owner

Choose a reason for hiding this comment

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

seems like this can be simplified to something like:

var ErrNotAStructPtr = errors.New("expected a pointer to a Struct")

as the message is always the same, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's definitely true, but I did it deliberately to follow the same error pattern like other errors (struct + func to create and error) do you recommend to make it simpler?

Copy link
Owner

Choose a reason for hiding this comment

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

yes, simpler is usually better :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I came to realize we are loosing a possibility to check the error type using a errors.New(). But anyway, I have updated this error, can you please check

error.go Outdated
Comment on lines 68 to 78
type NoParserError struct {
msg string
}

func newNoParserError(sf reflect.StructField) error {
return NoParserError{fmt.Sprintf(`no parser found for field "%s" of type "%s"`, sf.Name, sf.Type)}
}

func (e NoParserError) Error() string {
return e.msg
}
Copy link
Owner

Choose a reason for hiding this comment

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

I usually do something like this:

Suggested change
type NoParserError struct {
msg string
}
func newNoParserError(sf reflect.StructField) error {
return NoParserError{fmt.Sprintf(`no parser found for field "%s" of type "%s"`, sf.Name, sf.Type)}
}
func (e NoParserError) Error() string {
return e.msg
}
// NoParserError happens when etc...
type NoParserError struct {
Name string
Type string
}
func newNoParserError(sf reflect.StructField) error {
return NoParserError{sf.Name, sf.Type}
}
func (e NoParserError) Error() string {
return fmt.Sprintf("no parser found for field %q of type %q", e.Name, e.Type)
}

That way, internally, we can still get the "unparsed" name and type...

I would do this, and the same with the other similar errors bellow...

Also, we should add a comment to the error type saying when it should be raised and what it means.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair comment, I wanted to make all errors generic: the same structure (msg only), but your comment makes sense:

  • User converts error to specific type of the error
  • User now has access to all error fields +message string

Let me improve it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated as discussed

@pull-request-size pull-request-size bot added size/XL and removed size/L labels Jan 9, 2023
@pull-request-size pull-request-size bot added size/L and removed size/XL labels Jan 9, 2023
@akutuev
Copy link
Contributor Author

akutuev commented Jan 9, 2023

Hello @caarlos0. Can you please take a look at the changes when you have time

@akutuev
Copy link
Contributor Author

akutuev commented Feb 6, 2023

Hello @caarlos0, any updates on this? Do you need my help to clarify/review it?

@caarlos0
Copy link
Owner

caarlos0 commented Feb 6, 2023

sorry, got caught up in some personal issues, will review it when I get some peace time

@caarlos0 caarlos0 merged commit b135bbd into caarlos0:main Feb 7, 2023
caarlos0 added a commit that referenced this pull request Feb 7, 2023
refs #240

Signed-off-by: Carlos A Becker <caarlos0@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants