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 JSON InputSource to altsrc package #470

Merged
merged 2 commits into from Jul 5, 2016

Conversation

Projects
None yet
3 participants
@johnweldon
Contributor

johnweldon commented Jul 2, 2016

  • Implement NewJSONSource* functions for returning an InputSource from
    various JSON data sources.
  • Copy and modify YAML tests for the JSON InputSource
Add JSON InputSource to altsrc package
 - Implement NewJSONSource* functions for returning an InputSource from
   various JSON data sources.
 - Copy and modify YAML tests for the JSON InputSource
@johnweldon

This comment has been minimized.

Contributor

johnweldon commented Jul 2, 2016

Is travis-ci not completing the build because the pull request is against the v2 branch?

@jszwedko

This comment has been minimized.

Collaborator

jszwedko commented Jul 3, 2016

Hmm, I don't see why Travis didn't get or respond to the webhook for this PR. I pulled and pushed your changes just now so that we can follow https://travis-ci.org/urfave/cli/builds/141945098 for build status.

Thank you for adding JSON support! Reviewing now.

@jszwedko

This comment has been minimized.

Collaborator

jszwedko commented Jul 3, 2016

This looks good! Did you consider just unmarhaling into a map to reuse the MapInputSource code (similar to what the YAML altsrc does)? Just wondering if there were technical limitations as it would DRY this up a bit.

Build is green: https://travis-ci.org/urfave/cli/builds/141945098

Thanks again!

@johnweldon

This comment has been minimized.

Contributor

johnweldon commented Jul 3, 2016

@jszwedko thanks! Yes, I actually started with reusing the MapInputSource, but the json.Unmarshal doesn't work with map[interface{}]interface{}, only with map[string]interface{}

When I manually updated that top level to map[interface{}]interface{} after unmarshalling, I ran into type differences when using MapInputSource, I don't recall the specific errors, but for example, the json unmarshal treats all numbers as float64, which the MapInputSource couldn't handle when trying to get an int.

@jszwedko

This comment has been minimized.

Collaborator

jszwedko commented Jul 3, 2016

Ah, interesting, I do see that behavior regarding parsing all numbers as float64 which alone would make it difficult to use MapImportSource. Thank you for clarifying!

Will merge when the builds finish.

@jszwedko jszwedko merged commit 2b959bd into urfave:v2 Jul 5, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@johnweldon johnweldon deleted the johnweldon:PTAL-v2-implement-JSON-InputSource branch Jul 5, 2016

@anarcat

This comment has been minimized.

anarcat commented Feb 12, 2017

i'm a little confused - this looks like it was merged, but the v2 README doesn't mention JSON support at all... where did that patch go?

@jszwedko

This comment has been minimized.

Collaborator

jszwedko commented Feb 15, 2017

Hey @anarcat, this was merged into the v2 branch, which is intended to be the precursor to a breaking 2.0 release, but I see that there really isn't anything breaking here, so I'll cherry pick to master as a separate PR.

You are correct that it still needs to be added to the README though 😄

@jszwedko

This comment has been minimized.

Collaborator

jszwedko commented Feb 15, 2017

Opened #598 to backport.

@anarcat

This comment has been minimized.

anarcat commented Feb 15, 2017

well, the reason i'm confused is that the change to the README is in this PR (see 91563b8)

@jszwedko

This comment has been minimized.

Collaborator

jszwedko commented Feb 18, 2017

@anarcat indeed, I realized that when I went to port the feature to master. If you get a chance, would appreciate feedback on the PR (specifically if it works for your use cases!).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment