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 json.ParseWithStartPos function #389

Conversation

wata727
Copy link
Contributor

@wata727 wata727 commented Jun 10, 2020

Similar to hclsyntax.ParseConfig, this PR changes the start position to be passed to the json.Parse function.

This feature is necessary to transfer hcl.Block via RPC. We need to be able to specify the start position when we take a partial JSON and parse it on the other side. See also #332 #381

However, I understand that this change breaks backward compatibility. I propose this change for interface consistency, but if you care about compatibility, I could add a parsing function that accepts positions (e.g. json.ParseWithPos). Please tell me your opinion.

@apparentlymart
Copy link
Contributor

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

There are many callers of this function outside of the HCL codebase, so I think unfortunately we must retain API compatibility here, even though indeed the json package API is inconsistent with the others.

I like your proposed compromise of adding a new function. I'd suggest to call it ParseWithStartPos, to make it more obvious that the position argument is the starting position; I've seen some people be confused about the purpose of the pos argument in hclsyntax.ParseConfig so I think this is a good opportunity to learn from that feedback and make this new function name more explicit. We can make json.Parse be a wrapper around json.ParseWithStartPos using the default start position, so that there will still be only one real implementation of the function.

Does that sound reasonable to you? Thanks again!

@wata727 wata727 force-pushed the allow_to_pass_position_to_json_parse_function branch from c0d4ee5 to 3402913 Compare August 22, 2020 08:20
@wata727 wata727 changed the title json: Allow to pass hcl.Pos into Parse function json: Add json.ParseWithStartPos function Aug 22, 2020
@wata727
Copy link
Contributor Author

wata727 commented Aug 22, 2020

@apparentlymart Thank you for your review. Agreed with your suggestion.
Instead, I added the json.ParseWithStartPos function. Could you review it again?

@wata727 wata727 force-pushed the allow_to_pass_position_to_json_parse_function branch from 3402913 to ae3439e Compare August 22, 2020 09:22
@apparentlymart apparentlymart merged commit e72341d into hashicorp:hcl2 Aug 24, 2020
@apparentlymart
Copy link
Contributor

Thanks @wata727! I've merged it.

I'm going to wait a little while before tagging a new release because I'm currently in the review process for some other PRs and so I'd ideally like to include them all into the same release tag.

@freddd
Copy link

freddd commented Oct 1, 2020

Hi @apparentlymart, sorry to ping you in an old PR. Do you have any ETA on when you will be tagging a new release?

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