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

Add withContext to improve error reporting #194

Merged
merged 2 commits into from Apr 3, 2021
Merged

Conversation

johnynek
Copy link
Collaborator

@johnynek johnynek commented Apr 2, 2021

close #43

This follows the design sketched in the issue above. The goals are:

  1. not break compatibility
  2. not add a performance tax on those not using the feature
  3. enable users to add strings to track where they are in a parser.

cc @kubukoz @mpilquist

@codecov-io
Copy link

Codecov Report

Merging #194 (3b906eb) into main (f9bd3fa) will increase coverage by 0.54%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #194      +/-   ##
==========================================
+ Coverage   95.63%   96.18%   +0.54%     
==========================================
  Files           7        7              
  Lines         939      969      +30     
  Branches       79       96      +17     
==========================================
+ Hits          898      932      +34     
+ Misses         41       37       -4     
Impacted Files Coverage Δ
core/shared/src/main/scala/cats/parse/Parser.scala 95.87% <100.00%> (+0.65%) ⬆️

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 f9bd3fa...3b906eb. Read the comment docs.

Copy link
Member

@kubukoz kubukoz left a comment

Choose a reason for hiding this comment

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

Looks good to me! Although some test with an example and not as much logic as in the existing prop tests would be nice.

/** This is a reverse order stack (most recent context first)
* of this parsing error
*/
def context: List[String] =
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this tailrec? It'd probably require a reverse at the end but maybe it's worth pursuing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems to me if your errors are so nested you blow the stack here you will have other issues. I don't think it is worth a cost to retain the ability of having 50k deep nested contexts.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Fair. Let's leave it ;)

@johnynek johnynek merged commit 5159b61 into main Apr 3, 2021
@johnynek johnynek deleted the oscar/context_parser branch April 3, 2021 22:12
@johnynek
Copy link
Collaborator Author

johnynek commented May 7, 2021

This is published as part of 0.3.3

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.

design for scoping parsers for error reporting
3 participants