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

json: Add ParseExpression function #381

Merged
merged 1 commit into from
Sep 4, 2020

Conversation

wata727
Copy link
Contributor

@wata727 wata727 commented May 24, 2020

Fixes #332

Similar to hclsyntax.ParseExpression, This PR adds a function that parses a part of the JSON source.

Initially, I was looking for a way to transfer json.expression with RPC, but following the suggestion of @apparentlymart, I'm now thinking of sending text and parsing the text passed in each as an expression. This is a missing piece to achieve that.

Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

Hi @wata727! Thanks for working on this, and sorry for the slow response.

This looks good to me and I'd like to merge it. Would you mind adding in a few tests covering how it behaves when receiving values of some different types? I think the following would be a good set of inputs to test:

  • "hello" (a plain string)
  • "hello ${noun}" (a template string)
  • true and false
  • an unquoted JSON number
  • an empty JSON object and a JSON object with one or two properties in it.
  • an empty JSON array and a JSON array with one or two elements in it.

As an easy way to exercise these for testing, without having to write out verbose AST data structures to compare against, I suggest passing the test input to your new ParseExpression function to get an hcl.Expression, and then calling the Value on that expression with an EvalContext containing noun so that the template string above will work:

  got, diags := expr.Value(&hcl.EvalContext{
    Variables: map[string]cty.Value{
        "noun": cty.StringVal("world"),
    },
  })

The above should then allow the template string example to return cty.StringVal("hello world"), while the others should just return the cty type system equivalents of the literal values. If you print out the result values using %#v then it will show you how to write those values using the constructor functions in the cty package. I think that'll be easier to write and read than asserting against the hcl.Expression values directly, and the parser is already well-tested by the TestParse function.

json/public.go Outdated
@@ -62,6 +62,13 @@ func Parse(src []byte, filename string) (*hcl.File, hcl.Diagnostics) {
return file, diags
}

// ParseExpression parses the given buffer as a standalone JSON expression,
// returning it as an instance of Expression.
func ParseExpression(src []byte, filename string, start hcl.Pos) (hcl.Expression, hcl.Diagnostics) {
Copy link
Contributor

Choose a reason for hiding this comment

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

After your good idea in #389 I'd like to make this consistent with what we discussed there, by having ParseExpression take only the src and filename options and then have another function ParseExpressionWithStartPos that has the extra argument.

This is a tradeoff: given the current API we must decide to either make the json package API consistent with itself, or make this new function be consistent with the one of the same name in hclsyntax. Given those choices, I think it's better for the json package API to be consistent with itself, even though that makes it slightly inconsistent with hclsyntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good. I have added ParseExpression and ParseExpressionWithStartPos functions.

@wata727
Copy link
Contributor Author

wata727 commented Aug 22, 2020

@apparentlymart Thank you for your review and a kind explanation about how to write tests. I added tests to ParseExpression. Could you review it again?

@apparentlymart
Copy link
Contributor

Thanks for the additional updates, @wata727. This looks good to me and I'd like to merge it.

Unfortunately it seems like when I merged your other pull request I have created merge conflicts for the tests. 😖 I need to do some other work now so I will try to come back here and resolve the conflicts soon.

@wata727
Copy link
Contributor Author

wata727 commented Aug 25, 2020

@apparentlymart Thank you! Updated.

@apparentlymart
Copy link
Contributor

Thanks for resolving the conflicts, @wata727! I'm going to merge this now.

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.

2 participants