allow config to update based on specified path #1226
Conversation
…'s an attribute of the object)
|
Judging from the tests, this does not quite work yet; it fails already when trying to import qcodes. |
|
@WilliamHPNielsen the bug is a typo, I think you can get the main points from my comment or looking at the code and I was hoping for some feedback on the concept before I put time into testing. I'll assume this means you think the idea is fine? |
|
@nataliejpg and I was hoping for actually working code before I put time into reviewing... oh, no, now we're stuck in a deadlock 😄 |
|
@WilliamHPNielsen should work now |
qcodes/config/config.py
Outdated
| config = update(config, cwd_config) | ||
| self.config_file_path = path or self.config_file_path | ||
| if self.config_file_path is not None: | ||
| config_file = "{}/{}".format(self.config_file_path, self.config_file_name) |
There was a problem hiding this comment.
please use os.path.join instead of / and format. let the OS define the file separator.
There was a problem hiding this comment.
yup, that would be better, I just copied the original logic from likes 72 and 81 but will update them all now.
qcodes/config/config.py
Outdated
| cwd_config = self.load_config(self.cwd_file_name) | ||
| config = update(config, cwd_config) | ||
| self.config_file_path = path or self.config_file_path | ||
| if self.config_file_path is not None: |
There was a problem hiding this comment.
also, what about test? (see qcodes/tests/test_config.py)
There was a problem hiding this comment.
Unfortunately beyond my scope. perhaps someone (the author of the existing test?) could advise me what I need to add and I can put it in this pr? Otherwise realistically I won't be able to do this in the near future.
There was a problem hiding this comment.
The author is long gone, unfortunately. I agree that test_config.py is not the most immediately readable piece of code. I can help you extend the test to cover the new functionality introduced in this PR.
There was a problem hiding this comment.
thanks @WilliamHPNielsen. How about I test this out when I'm next measuring and then we merge it and @WilliamHPNielsen writes the test in another pr for the actual test?
There was a problem hiding this comment.
just don't throw away the code that you will be writing when you will be "testing this out when you're next measuring" ;)
| self.validate(config, self.current_schema, | ||
| self.schema_cwd_file_name) | ||
|
|
||
| self.current_config = config | ||
|
|
There was a problem hiding this comment.
I suppose we still want this method to return the config?
There was a problem hiding this comment.
why? I guess it would work better in terms of backwards compatibility (if anyone is actually using the update function) but to me it seems a bit weird to have an update function that doesn't actually update the current_config (the last one didn't)
There was a problem hiding this comment.
this depends on how people use it. Unfortunately, docs/examples/Configuring_QCoDeS.ipynb does not shed light on it... I leave this decision to you.
There was a problem hiding this comment.
The missing return is the reason that the tests fail. I think we need a pretty strong argument for breaking/changing behaviour that we explicitly test for. We don't have visibility on who uses the software and how, so the golden rule is that we don't suddenly change the API.
But I agree that the old/current API is not very intuitive, so we might deprecate some functions in favour of new ones that act better.
|
I guess I volunteered to clean this up. I am going to push one or two commits now, fixing the return and extending the tests. Another issue is improving the documentation... |
|
thanks @WilliamHPNielsen |
Codecov Report
@@ Coverage Diff @@
## master #1226 +/- ##
==========================================
+ Coverage 80.5% 80.53% +0.02%
==========================================
Files 49 49
Lines 6807 6816 +9
==========================================
+ Hits 5480 5489 +9
Misses 1327 1327 |
|
Note to @QCoDeS/core: |
Fixes #1223 .
allows a config file path to be specified either in initialisation of the Config object or when running update. This path is then used for future updates.
How's this @jenshnielsen? I haven't tested it yet.