Add Subscribers from config #1365
Conversation
Dominik-Vogel
requested review from
jenshnielsen,
astafan8 and
WilliamHPNielsen
Codecov Report
@@ Coverage Diff @@
## master #1365 +/- ##
==========================================
+ Coverage 73.08% 73.15% +0.06%
==========================================
Files 79 79
Lines 9134 9155 +21
==========================================
+ Hits 6676 6697 +21
Misses 2458 2458 |
|
Should the plottr link be to https://github.com/kouwenhovenlab/plottr or https://github.com/wpfff/plottr ? |
|
|
||
| VALUE = Union[str, Number, List, ndarray, bool] | ||
|
|
||
|
|
||
| class TestSubscriber(): |
There was a problem hiding this comment.
I would suggest not calling this class Test... since it's not actually a test. \
This works fine at the moment because pytest only discovers methods starting with test inside such a class and only if there is no init method but it's still potentially confusing
| @contextmanager | ||
| def default_config(): | ||
| def default_config(user_config: Optional[str]=None): | ||
| """ | ||
| Context manager to temporarily establish default config settings. | ||
| This is achieved by overwritting the config paths of the user-, | ||
| environment-, and current directory-config files with the path of the | ||
| config file in the qcodes repository. |
There was a problem hiding this comment.
do empty strings assigned below to all the file name attributes mean the same as "the path of the config file in the qcodes repository"? I am not sure i'm following what's going on here but at least either the code change is too much or the docstring needs to be adjusted as well.
There was a problem hiding this comment.
Maybe these can be None but it is all good as long as it is not a filename. I chose the empty string to be on the safe side with typing.
There was a problem hiding this comment.
ok, implementation is fine. My worry is about consistency of the docstring with the code. Now it claims smth that is not happening in the code, namely "overwritting the config paths of the user-, environment-, and current directory-config files with the path of the config file in the qcodes repository". What's happening now is "overwritting the config paths of the user-, environment-, and current directory-config files with empty strings", right?
Co-Authored-By: Dominik-Vogel <[email protected]>
…s into default_subscription
Merge: 43af32c 393498c Author: Dominik Vogel <[email protected]> Merge pull request #1365 from Dominik-Vogel/default_subscription
| qcodes.config.core.db_location = db_location | ||
|
|
||
| assert 'test_subscriber' not in qcodes.config.subscription.subscribers | ||
| with pytest.raises(RuntimeError): |
There was a problem hiding this comment.
hope that it's fine now, but I recommend next time to also assert the exception message because there are many RuntimeErrors while we are looking for a very specific one.
There was a problem hiding this comment.
I agree. I was running out of time and the test should go in before a certain internal release yesterday.
This PR enables the creation of subscribers for e.g. live plotting by adding them to the config.
There are quite many issue concerning the subscription (i.e. accessing data from different subscriber threads). These are not addressed here :-(.
Take a look at the sample notebook :-)