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

Adjust JSON handling to be like RDS #200

Merged
merged 1 commit into from Feb 11, 2019

Conversation

Projects
None yet
3 participants
@nathancday
Copy link
Contributor

nathancday commented Feb 9, 2019

After digging into the code I pivoted to adjusting handling of JSON file formats in import() and export() instead of in import_list(). My thought process was that because the underlying jsonlite functions don't have a which/itemizer to specify a sub unit , like XLS sheets, nor are the objects saved in itemized way like Rdata, the import_list() function didn't really fit. But since toJSON() and fromJSON() already handle a variety of R object classes I figured the current pattern for RDS objects was most similar to the concept of jsonlite. I realize this is a detour from the original issue I raised about import JSON lists #199 but it seems to fit with issue #183. Thoughts?

@nathancday nathancday force-pushed the nathancday:master branch from b9c5e51 to 5d705bb Feb 9, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 9, 2019

Codecov Report

Merging #200 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #200   +/-   ##
=======================================
  Coverage   81.99%   81.99%           
=======================================
  Files          18       18           
  Lines         861      861           
=======================================
  Hits          706      706           
  Misses        155      155
Impacted Files Coverage Δ
R/import.R 91.66% <100%> (ø) ⬆️
R/export.R 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32662b6...25b8546. Read the comment docs.

@nathancday nathancday force-pushed the nathancday:master branch from 5d705bb to 25b8546 Feb 9, 2019

@leeper

This comment has been minimized.

Copy link
Owner

leeper commented Feb 10, 2019

On glance, this looks good. Will examine more closely a little later. Thanks!

@leeper leeper added the enhancement label Feb 11, 2019

@leeper leeper merged commit 25b8546 into leeper:master Feb 11, 2019

4 checks passed

codecov/patch 100% of diff hit (target 81.99%)
Details
codecov/project 81.99% (+0%) compared to 32662b6
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

leeper added a commit that referenced this pull request Feb 11, 2019

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