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 section for error handling #116

Closed
wants to merge 1 commit into from

Conversation

sebastien-rosset
Copy link
Contributor

@sebastien-rosset sebastien-rosset commented Mar 19, 2023

This PR adds a section about error handling.

The following statement is written twice inside the Function Evaluation section:

How and when this error is raised is implementation specific

IMHO, this statement applies to all error handling scenarios. So instead of making that statement specific to function evaluation, it could be moved to the section about error handling.

The spec has 34 occurrences of the word "error", but it does not really codify how errors are raised.
The spec does not have a list of error types and it does not have guidelines about when to use these error types.

Copy link
Contributor

@springcomp springcomp left a comment

Choose a reason for hiding this comment

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

Please, include not-a-number and unknown-function (on the top of my head).

@springcomp
Copy link
Contributor

springcomp commented Mar 21, 2023

@sebastien-rosset thanks for your contribution.

I think this is a good opportunity to address error type precedence in this PR.

Also, any changes to the spec should ideally be tracked into a JEP – only for historical purposes.
You can try submitting a discussion / JEP to this repo ? If you feel comfortable, we are happy to discuss this in JMESPath Community as well.

Would you mind starting a JEP document in the discussions for that? You can find some samples of past discussions tagged with jep-candidate there as well.

The numbering would be defined later on.

Copy link
Member

@jamesls jamesls left a comment

Choose a reason for hiding this comment

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

The compliance tests repo defines these error types so I'm fine merging as is. The compliance tests also define a syntax error type, but I don't think we need that as it's implied by the grammar that anything that doesn't parse properly raises a syntax error.

Thanks again for the contribution!

jamesls added a commit that referenced this pull request Mar 24, 2023
PR #116

* error-section:
  Add section for error handling
@jamesls
Copy link
Member

jamesls commented Mar 24, 2023

Rebased/merged in 28fd853

@jamesls jamesls closed this Mar 24, 2023
springcomp added a commit to jmespath-community/jmespath.spec that referenced this pull request Mar 25, 2023
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.

None yet

3 participants