-
Notifications
You must be signed in to change notification settings - Fork 98
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
Tests for custom offset window #1585
base: court/custom-offset6.5
Are you sure you want to change the base?
Conversation
f5b1e3a
to
a719cb6
Compare
0048a87
to
33fe95d
Compare
e91fd30
to
7e13cf3
Compare
118b710
to
5455fe5
Compare
d0c473a
to
bebe99d
Compare
3d9de58
to
b53a655
Compare
d260a66
to
75042dd
Compare
ac725b7
to
ff3d3f9
Compare
eda848f
to
1e2b931
Compare
1e2b931
to
7ba3e07
Compare
7ba3e07
to
35c74fd
Compare
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 though please see comment about martian_time
.
docstring: | ||
Gives a side by side comparison of bookings and bookings_offset_one_martian_day. | ||
--- | ||
metric_time__martian_day metric_time__day bookings bookings_offset_one_martian_day |
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.
- With how
martian_day
is defined, it's a little hard to check these snapshots for correctness. - A more straightforward mapping would help e.g. martian days are always 10 years before.
- Also time on Mars is represented differently externally, so a different name would help.
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.
Since it's not totally relevant to this PR, I'm going to update this at the top of the stack if you don't mind!
ff3d3f9
to
e073215
Compare
a6ad1d6
to
49c5ce6
Compare
3340fd8
to
b12476e
Compare
49c5ce6
to
4ad304c
Compare
Tests for custom offset window. The snapshots in this PR should give you a better idea of what the SQL and output should look like.