Skip to content

Improve Error system #70

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

Jump to bottom
Merged
merged 9 commits into from
Jul 12, 2022
Merged

Improve Error system #70

merged 9 commits into from
Jul 12, 2022

Conversation

gsempe
Copy link

@gsempe gsempe commented Dec 18, 2021

As a library user, we needed a way to translate error messages coming from the library.
This pull request introduces a way to let library users customize error messages if they want. It is as well fully retro-compatible as it doesn't change the current error messages.

What the pull request changes:

  • Make the error type public
  • Add an error code to let library users identify the error reason
  • Add a Reason attribute for errors cases that need the name of the entity generating the error
  • Make the Line, Code and Reason attribute public to let library users customize (translate) error message
  • Keep the current error messages

@gsempe
Improve error type
be1b252 
Add the error type to the public interface and add a code for each
different type of error to let library users translate errors if they
need to.
@gsempe
Improve error system
3177484 
Add type for the error code and documentation
@gsempe
Improve documentation
71df958
@gsempe
Fix error default message
66b9596
@gsempe
Copy link
Author

gsempe commented Jan 17, 2022

Checking how this change proposal is received.
I hope it will make sense as I feel it is something several users of the library can benefits from, it is retrocompatible and it is not making the interface more complicated.

@cbroglie
Copy link
Owner

Thanks for your contribution @gsempe, I haven’t had a chance to review it yet

@gsempe
Copy link
Author

gsempe commented Jan 18, 2022

No worries, I'm at your disposal if you need anythiing

@gsempe
Copy link
Author

gsempe commented Jun 13, 2022

Hi @cbroglie, I'm checking on my different dependencies and I was thinking to ask for news from the project.
Can I help you?

cbroglie
cbroglie requested changes Jun 14, 2022
View reviewed changes
Copy link
Owner

@cbroglie cbroglie left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please include a test case using errors.As to extract a ParseError and verify its fields. Looking good otherwise!

cbroglie
cbroglie reviewed Jun 14, 2022
View reviewed changes
@gsempe
Copy link
Author

gsempe commented Jun 14, 2022

I have added a new test on ParseError unwrapping based on the malformed tests. What do you think about this new test?

@cbroglie
Copy link
Owner

The build is failing here due to issues resolved in #78, do you mind rebasing this with latest master?

@gsempe
Copy link
Author

gsempe commented Jun 22, 2022

The build is failing here due to issues resolved in #78, do you mind rebasing this with latest master?

Done 🙌

@codecov-commenter
Copy link

Codecov Report

Merging #70 (96e077c) into master (3fabeb4) will decrease coverage by 0.48%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
- Coverage   82.56%   82.08%   -0.49%     
==========================================
  Files           2        3       +1     
  Lines         654      681      +27     
==========================================
+ Hits          540      559      +19     
- Misses         86       94       +8     
  Partials       28       28
Impacted Files Coverage Δ
mustache.go 83.33% <42.85%> (-0.06%) ⬇️
error.go 72.41% <72.41%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3fabeb4...96e077c. Read the comment docs.

@cbroglie cbroglie merged commit c1e7de1 into cbroglie:master Jul 12, 2022
@cbroglie
Copy link
Owner

Thanks @gsempe, I released v1.4.0 with these changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbroglie cbroglie cbroglie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gsempe @cbroglie @codecov-commenter