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

fix JSON format for list input #174

Merged
merged 1 commit into from
Apr 21, 2017
Merged

fix JSON format for list input #174

merged 1 commit into from
Apr 21, 2017

Conversation

pieterprovoost
Copy link
Contributor

This fixes some issues I had with the JSON produced in make_bulk() for list input:

  • NA becomes "NA" which is an issue particularly for non character fields
  • atomic vectors are not unboxed

This may break things for other users, please evaluate.

@codecov-io
Copy link

codecov-io commented Apr 20, 2017

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #174   +/-   ##
=======================================
  Coverage   63.84%   63.84%           
=======================================
  Files          37       37           
  Lines        1300     1300           
=======================================
  Hits          830      830           
  Misses        470      470
Impacted Files Coverage Δ
R/docs_bulk_utils.R 77.94% <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 5f2b557...dd563d1. Read the comment docs.

@sckott
Copy link
Contributor

sckott commented Apr 20, 2017

thanks @pieterprovoost will have a look and see if this will be okay, does make sense to set NA's to null for json

@sckott
Copy link
Contributor

sckott commented Apr 21, 2017

seems good, except noticed that it has the effect that your change leads to all fields being retained in ES with null's retained, while on master now ES just drops those fields that have NA's - i imagine meaning data in ES is smaller with version on master now.

i'll note that null values can't be searched https://www.elastic.co/guide/en/elasticsearch/reference/5.3/null-value.html

i'll add some tests for NA's in data.frame's, lists, and files

@sckott sckott added this to the v0.8 milestone Apr 21, 2017
@sckott sckott merged commit e25cb90 into ropensci:master Apr 21, 2017
sckott added a commit that referenced this pull request Apr 21, 2017
…values

for docs_bulk
instsall covr on travis only
bumped version
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