Add support for table decorating methods (snapshot() and window()). #331
Conversation
|
|
||
| return int(value) | ||
|
|
||
| def at(self, when): |
There was a problem hiding this comment.
Seems like having a couple methods snapshot and window might cleanup the two cases of fixed snapshot and relative snapshot and make it easier to understand what is happening without a length doc-comment on the when arg. Thoughts?
There was a problem hiding this comment.
I think snapshot could take an optional end value, and subsuming the between functionality.
There was a problem hiding this comment.
window implies start/end bounds, so there is still the case of a single relative argument (i.e. a relative snapshot, not interval).
Maybe:
- window(start, end) for relative interval
- between(start, end) for absolute interval
- at(when) for absolute snapshot
- ago(when) for relative snapshot
Does that sound better? at() and ago() seem fairly clear; I'm less happy with the other two. We could add a second argument to at() for an interval and that is still reasonable, although I'm less sure about ago().
Another more verbose option might be:
- snapshot_at(when) - absolute
- snapshot_ago(when) - relative
- window_at(start, end)
- window_ago(start, end)
That's different semantics to what you suggested for snapshot and window, but again I think window makes more sense for a range, while snapshot makes sense for a point in time.
Yet another option:
fixed_window(start, end)
sliding_window(start, end)
fixed_snapshot(when)
sliding_snapshot(when)
or perhaps the most obvious, if ugly:
absolute_window()
relative_window()
absolute_snapshot()
relative_snapshot()
There was a problem hiding this comment.
I think the API should reflect the two scenarios in its naming/docs, so I'd use those terms.
snapshot of data at a point in time:
snapshot(datetime.datetime) -- absolute
snapshot(datetime.timedelta) -- relative [to now]
window of data over an interval of time:
window(datetime.datetime, datetime.datetime) -- absolute
window(datetime.timedelta) -- relative [to now]
Will that work?
There was a problem hiding this comment.
I assume your last one should have had two timedelta arguments.
I'm happy with that.
There was a problem hiding this comment.
No, I intended for only one. The common case (last hour for example) only needs one timedelta. For something like last day to last hour, I was thinking the user would convert to absolute datetime themselves. I preferred the symmetry with snapshot.
That said, I am not averse to adding it, but it should be optional.
|
Added a few comments on the APIs themselves. |
|
PTAL |
gramster
changed the title
|
|
||
| return int(value) | ||
|
|
||
| def snapshot(self, when): |
There was a problem hiding this comment.
Minor suggestion... use it if it makes sense/reads better to you as well.
when -> at
|
Some minor naming/comments related comments. |
|
Been waiting on am LGTM - looking back at this the comments were minor and are addressed so I'm going to merge. |
Add support for table decorating methods (snapshot() and window()).
* Set up tox, and use it for local testing and Travis. [tox](https://tox.readthedocs.io/) is a Python test automation tool: it makes it easy to automate running the tests for your project in several environments. Under the hood, it just uses virtualenv to manage installed deps. If you have python2.7 and python3.5 installed, simply running `tox` at the root of the repo will now run tests in both versions, as well as `flake8`. This PR makes two changes: * first, convert the existing `.travis.yml` into `tox.ini`, setting up three tox environments based on the configuration that was already there. (Relatedly, I added the `.tox` directory to `.gitignore`). * Now that googledatalab#330 is fixed (wooo @brandondutra), I switched the `.travis.yml` over to use `tox`. Now there are separate travis environments for each tox environment, which has two advantages: (1) these three all run in parallel, speeding up builds, and (2) build logs are separated, making it faster to figure out what caused a given failure. * (Code review followups, delete message on squash) 1. Note in `CONTRIBUTING.md` 2. Simplified `flake8` invocation in `tox.ini`. * (more code review) * (last code review round)
No description provided.