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

adding support for --wrapScalar=false in properties encoder #1241

Merged
merged 5 commits into from
Jun 25, 2022

Conversation

dcarbone
Copy link
Contributor

I'm currently running into a scenario where it would be useful to wrap the value side of the output from the Properties encoder.

This PR is a first draft at that with some tests, please let me know if you'd like some more!

@mikefarah
Copy link
Owner

This looks really good - you just need to add test(s) into properties_test.go, this will also generate documentation so others know about the feature 👍🏼

@dcarbone dcarbone force-pushed the dcarbone/properties-wrapscalars branch from 463ba68 to adf8a51 Compare June 17, 2022 22:36
@dcarbone
Copy link
Contributor Author

dcarbone commented Jun 18, 2022

@mikefarah i've updated the test workflow a bit and added one for my added feature.

if you disagree with the changes, i can work on a different solution.

(bad internet, sorry for the double comment)

Additionally, I would like to potentially take a stab at refactoring a bit of the test suite, but could do that in a separate PR.

@mikefarah
Copy link
Owner

Thank you! There is a problem though in the documentation generated, it doesn't show the updated command to get the wrapping (e..g supplying --unwrapScalar=false - if you have a look at xml_test.go, you can see that there are added 'scenarioType's that generate a slightly different documentation on the command, as well us parameterising the encoder/decoder correctly.

@dcarbone dcarbone force-pushed the dcarbone/properties-wrapscalars branch from 1efe86d to 23be0b0 Compare June 24, 2022 02:49
@dcarbone
Copy link
Contributor Author

dcarbone commented Jun 24, 2022

@mikefarah sorry it took a few days to get back to you on this! i reverted my initial pass as it wasn't really in keeping with your original design, and i didn't want to start imposing my own opinions there.

with this update, i've done the following:

  1. replaced the various if .. else .. cases when processing scenarios with a switch
    • this was mostly to make it a smidge cleaner to read and expand upon, and also forces future scenarioTypes to have an explicit handler, rather than letting it succeed with ambiguous correctness.
  2. replaced a few var definitions with const as their values are indeed constant
  3. added a new scenario type (encode-wrapped) for the properties test suite.

let me know what you think :)

@mikefarah mikefarah merged commit 98b411f into mikefarah:master Jun 25, 2022
@dcarbone dcarbone deleted the dcarbone/properties-wrapscalars branch June 25, 2022 02:22
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