-
Notifications
You must be signed in to change notification settings - Fork 65
Fix bugs in Unflattening Data #930
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
Conversation
try: | ||
key = int(part) | ||
except ValueError: | ||
key = part |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could prob do if key.isnumeric()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part will also be a str
though? So I would have to cast it to make it work I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isnumeric is a string method to tell you if all the characters are digits. so you could do
if part.isnumeric():
key = int(part)
else:
key = part
I just like to avoid extraneous try/catches where possible. maybe that's silly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that makes sense. I can do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait isdigit does the same thing. Why did we change this from the prev implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because both isdigit
and (I just realized) isnumeric
don't handle decimals or negative numbers. I'll stick with the current implementation for now.
lib/sycamore/sycamore/connectors/elasticsearch/elasticsearch_writer.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Fixes bugs in unflattening data and comparing documents and the arising broken tests and bugs in various connectors.