Closes #60 #63
Closes #60 #63
Conversation
There was a problem hiding this comment.
Looks good to me. @acs @valeriocos could you you have al lpok, please?
| start_ts = start.replace(tzinfo=timezone.utc).timestamp() | ||
| start_ts_ms = start_ts * 1000 # ES uses ms | ||
| if end: | ||
| end = end.replace(microsecond=0) |
There was a problem hiding this comment.
Why do we need to set microsecond to 0? Is not configured in this way by default?
There was a problem hiding this comment.
@acs: No, it isn't configured this way by default and results in an error from elasticsearch.
This error presents it self when we convert datetime to timestamp.
I'll update the file and mention it there.
There was a problem hiding this comment.
Ah, ok, it is related with the migration to elasticsearch_dsl. Great! I want just to understand the need for it. Thanks you @aswanipranjal ! LGTM
manuscripts/metrics/metrics.py
Outdated
| response = s.execute() | ||
| return response.to_dict() | ||
| except Exception as e: | ||
| raise e |
There was a problem hiding this comment.
Capturing the Exception an reraising it without doing any thing is a bit weird. And having the return inside the try also.
There was a problem hiding this comment.
My bad. I wanted to use the error and show the exact information about the error to the user to make debugging easier.
There was a problem hiding this comment.
IMHO once this comment is addressed the PR is ready to be merged.
aswanipranjal
force-pushed
the
cleanup-esquery2
branch
from
c656691
to
c79656a
Compare
|
PR #64 and this one have similar errors. I am looking into them. |
|
So, for the tests, what is happening is: In #64 i removed the error from |
aswanipranjal
force-pushed
the
cleanup-esquery2
branch
from
c79656a
to
f67bf07
Compare
|
Fixed the errors using OrdereDict instead. |
|
I see that fixed the test. So, ready to go! |
Updated comments in esquery.py.
Metrics.py uses elasticsearch_dsl.py instead of directly requesting easticsearch for data.
Updated test for get_agg.