Skip to content

Conversation

@samiam2013
Copy link
Collaborator

@samiam2013 samiam2013 commented Aug 3, 2025

Summary

Implements the typed errors feature discussed in #111 using the errors.Join() approach. This provides type-safe error checking while maintaining full backwards compatibility.

Key Features

  • New -typederrors flag: Optional flag that enables typed error generation (defaults to false)
  • Custom error type: enumerrs.ErrInvalidValue sentinel error for type-safe checking
  • errors.Join() approach: Combines typed error with descriptive messages
  • Full backwards compatibility: Existing code continues working unchanged

Implementation Details

  • With -typederrors: Returns joined typed error errors.Join(enumerrs.ErrInvalidValue, fmt.Errorf("..."))
  • Without flag: Returns standard fmt.Errorf(...) (unchanged behavior)
  • All marshaling methods supported: JSON, YAML, Text unmarshaling all use typed errors when enabled
  • Test coverage: All existing tests pass, new functionality verified

Test Plan

  • Verify typed error detection with errors.Is()
  • Verify backwards compatibility without flag
  • Verify descriptive error messages preserved
  • Verify all existing tests continue to pass
  • Verify JSON/YAML/Text unmarshaling uses typed errors when enabled
  • New Golden Test
  • New github CI test flows for e2e & golden testing

🤖 Generated with help from Claude Code

@samiam2013 samiam2013 requested a review from Copilot August 3, 2025 06:51

This comment was marked as outdated.

samiam2013

This comment was marked as spam.

@samiam2013 samiam2013 requested a review from Copilot August 3, 2025 06:59

This comment was marked as outdated.

@samiam2013

This comment was marked as spam.

@samiam2013 samiam2013 requested a review from Copilot August 3, 2025 07:10

This comment was marked as spam.

@samiam2013
Copy link
Collaborator Author

Thank you for your review @bincyber I plan on rebasing and adding you as a contributor for your efforts here unless you object. I think in addition to the change to a single shared definition of this error it should be named more idiomatically, if I'm remembering correctly that means ErrInvalidValue with the err first since it's a value and primarily an error.
I appreciate your enthusiasm for this change and I will be trying to find the time to fit this in but you should know my best friend IRL has some things I need to help with and it will be a juggling act to fit this into the rotation

@samiam2013 samiam2013 marked this pull request as ready for review August 4, 2025 19:58
@bincyber
Copy link
Contributor

bincyber commented Aug 5, 2025

Thank you for your review @bincyber I plan on rebasing and adding you as a contributor for your efforts here unless you object. I think in addition to the change to a single shared definition of this error it should be named more idiomatically, if I'm remembering correctly that means ErrInvalidValue with the err first since it's a value and primarily an error. I appreciate your enthusiasm for this change and I will be trying to find the time to fit this in but you should know my best friend IRL has some things I need to help with and it will be a juggling act to fit this into the rotation

Sounds great

@samiam2013 samiam2013 force-pushed the samiam2013/typed-errors branch from 02356a8 to a8b6e3c Compare August 6, 2025 13:30
@samiam2013
Copy link
Collaborator Author

samiam2013 commented Aug 6, 2025

I went to add this error globally and found it can't be at the top of the repo because that's where the tool code lives and we would have to make enumer itself a depdendency of the generated code. I was able to sort an implementation of the change using strconv.ErrRange.

One alternative for having the error in scope without adding a dependency is to de-duplicate: we could write just a separate file with the error type (typeS in the future) and check if it's there before writing again from another type being generated.

This has downsides like the errors could be duplicated across a codebase in different paths and even just different levels of hierarchical data structures. It would also be a more significant refactor.

I think the path of a new package space inside enumer, github.com/dmarkham/enumer/errs is the least-friction path ahead and because this addition of a generated non-standard-library dependency is behind a flag that's newly added I don't think it represents the concern it felt like when I uncovered that requirement.

I also need to remember to update the README with this new flag

@samiam2013 samiam2013 force-pushed the samiam2013/typed-errors branch 3 times, most recently from 2296974 to 6678004 Compare August 7, 2025 05:22
Copy link
Contributor

@bincyber bincyber left a comment

Choose a reason for hiding this comment

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

🚢

@samiam2013
Copy link
Collaborator Author

One last thing: like #111 I think this deserves a "golden" test for the generated return error

@samiam2013 samiam2013 force-pushed the samiam2013/typed-errors branch 2 times, most recently from 701fa38 to 2d32fa8 Compare August 12, 2025 04:10
@samiam2013 samiam2013 requested a review from Copilot August 12, 2025 05:17

This comment was marked as outdated.

@samiam2013 samiam2013 force-pushed the samiam2013/typed-errors branch from 477c248 to 95f9918 Compare August 12, 2025 05:37
@samiam2013
Copy link
Collaborator Author

because this is an addition but is backwards compatible, I think the new tag should be version 1.6.0

Co-authored-by: @bincyber <bincyber@users.noreply.github.com>
@samiam2013 samiam2013 force-pushed the samiam2013/typed-errors branch from 95f9918 to 599be04 Compare August 12, 2025 06:02
@samiam2013
Copy link
Collaborator Author

I keep thinking about how to test this further and because the referenced error is in this change, I can't do too much. I know it's working because of the testing, and I would have noticed the other .golden test files getting broken if it broke backwards compatibility. I'm just going to merge it.

@samiam2013 samiam2013 merged commit 92223cb into master Aug 15, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants