Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign uphardened handling of aliases #73
Conversation
jameslamb
force-pushed the
jameslamb:bugfix/get_fields_aliases
branch
from
62dca64
to
1b8691e
|
fun fact ES5.x onwards and legacy ES versions return alias results slightly differently. Added a fix to this PR. I'm glad we caught it before the release |
jameslamb
force-pushed the
jameslamb:bugfix/get_fields_aliases
branch
from
1b8691e
to
9989e04
Codecov Report
@@ Coverage Diff @@
## master #73 +/- ##
==========================================
+ Coverage 86.08% 86.93% +0.84%
==========================================
Files 9 9
Lines 611 643 +32
==========================================
+ Hits 526 559 +33
+ Misses 85 84 -1
Continue to review full report at Codecov.
|
| , mappingDT = mappingDT | ||
| ) | ||
| , fill = TRUE | ||
| ) |
austin3dickey
Jun 18, 2018
Member
I think there's a better way you could do this with a data.table merge but these DTs are small anyway so it probably doesn't matter
I think there's a better way you could do this with a data.table merge but these DTs are small anyway so it probably doesn't matter
jameslamb
Jun 18, 2018
Author
Member
yeah they're pretty small and this makes it really explicit. This is an EDA function so super-super fast performance is less important than correctness, IMHO
yeah they're pretty small and this makes it really explicit. This is an EDA function so super-super fast performance is less important than correctness, IMHO
|
LGTM |
| return(.process_alias(alias_string = resultContent)) | ||
| major_version <- .get_es_version(es_host) | ||
| process_alias <- switch( | ||
| major_version |
ngparas
Jun 18, 2018
•
Collaborator
since there are also minor changes in scrolling between the versions i'd love if we could pull all the version-specific stuff up higher and configure w/ passing functions instead of putting internal switches like this
doesn't have to be a blocker for now though since thats a pretty large and maybe unnecessary change for our purposes
since there are also minor changes in scrolling between the versions i'd love if we could pull all the version-specific stuff up higher and configure w/ passing functions instead of putting internal switches like this
doesn't have to be a blocker for now though since thats a pretty large and maybe unnecessary change for our purposes
jameslamb
Jun 18, 2018
Author
Member
fair point! I will throw that up in an issue
fair point! I will throw that up in an issue
Sorry @austin3dickey , one more that I want to go out on the v0.3.0 release.
We don't have any tests right now on how aliases are handled in
get_fields. This PR addresses that.One thing to call out is that having multiple aliases point to the same index is a totally valid thing, and our code previously couldn't handle that. This PR adds that support as well.
One other minor bugfix...
urlis a base function, so I changed our internal references toes_url. I noticed it because I was getting the dreadedclosure not subsettableerror while debugging.