-
Notifications
You must be signed in to change notification settings - Fork 301
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
Feature: access snapshot of a DataSet, and docs (also, Station bug fix + tests) #1322
Merged
astafan8
merged 35 commits into
microsoft:master
from
astafan8:feature/snapshot_docs_and_tests
Oct 19, 2018
Merged
Feature: access snapshot of a DataSet, and docs (also, Station bug fix + tests) #1322
astafan8
merged 35 commits into
microsoft:master
from
astafan8:feature/snapshot_docs_and_tests
Oct 19, 2018
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
the DataSet.get_metadata('snapshot') method is not mentioned at all, because it is actually dangerous.
astafan8
requested review from
jenshnielsen,
WilliamHPNielsen and
Dominik-Vogel
October 16, 2018 12:16
1 task
as opposed to exist method, this method works on instances and hides all the related logic.
If an instrument is a part of a station, and it was closed during the lifetime of that station, the snapshot function used to fail with a cryptic exception. Now this is fixed. When a station snapshot is made, instruments that are closed (not valid) do not get snapshotted, and are automatically removed from the station.
astafan8
changed the title
Feature: access snapshot of a DataSet, and docs
Feature: access snapshot of a DataSet, and docs (also, Station bug fix + tests)
Oct 17, 2018
Codecov Report
@@ Coverage Diff @@
## master #1322 +/- ##
=========================================
+ Coverage 72.32% 72.5% +0.17%
=========================================
Files 74 74
Lines 8525 8560 +35
=========================================
+ Hits 6166 6206 +40
+ Misses 2359 2354 -5 |
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.
Left a few very minor comments but looks good otherwise
jenshnielsen
approved these changes
Oct 19, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's in:
DataSet.snapshot
andDataSet.snapshot_raw
(taken from [wip] Dataset serializer #1262 PR) properties to retrieve snapshot dict and JSONStation
object is now covered with tests, also introduces useful fixture that cleans upStation.default
at startup and teardown of a testStation.snapshot
would raise a cryptic exception in case an instrument that is a part of the station has been closed; now, snapshot method proceeds, and the "closed" instrument is removed from the station automatically; this partially addresses problems raised in Instrument.close() #277.Some of the code from this PR was already in #1215 PR, but that one stalled for a bit in WIP mode, hence this new PR.