Skip to content
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

Config cleanup #1242

Merged
merged 7 commits into from
Aug 28, 2018
Merged

Config cleanup #1242

merged 7 commits into from
Aug 28, 2018

Conversation

jenshnielsen
Copy link
Collaborator

@jenshnielsen jenshnielsen commented Aug 22, 2018

A few changes to the config system to hopefully make it more logical

  • load_config no longer has the side effect of setting the path to
    the config file as it doesn't actually update the current_config
    but just returns the config loaded from the file
  • Make custom path behave like all other configs but load it last
    so it takes preference over the other config files
  • Change __repr__ to include the path of all config files used
  • Refactor to reduce code duplication
  • Correct a few typos

current_config_file is still there and points to the last config file
loaded as it did before. This is perhaps confusing? Do we want to
remove it altogether.

@WilliamHPNielsen I think this will solve your problem.

@nataliejpg Any issue with the change to path loading logic. The path specified file will still overwrite anything else

EDIT by William: This fixes #1229

jenshnielsen and others added 2 commits August 22, 2018 15:45
* load_config no longer has the side effect of setting the path to
   the config file as it doesn't actually update the current_config
   but just returns the config loaded from the file
* Make custom path behave like all other configs but load it last
   so it takes preference over the other config files
* Change `__repr__` to include the path of all config files used
* Refactor to reduce code duplication
* Correct a few typos

current_config_file is still there and points to the last config file
loaded as it did before. This is perhaps confusing? Do we want to
remove it altogether.
@astafan8
Copy link
Contributor

Looks very nice!

Two questions:

@codecov
Copy link

codecov bot commented Aug 22, 2018

Codecov Report

Merging #1242 into master will increase coverage by <.01%.
The diff coverage is 91.3%.

@@            Coverage Diff             @@
##           master    #1242      +/-   ##
==========================================
+ Coverage   79.16%   79.17%   +<.01%     
==========================================
  Files          52       52              
  Lines        7201     7199       -2     
==========================================
- Hits         5701     5700       -1     
+ Misses       1500     1499       -1

@jenshnielsen
Copy link
Collaborator Author

Not sure about tests. Changes in functionality are very limited. The only user visible change is probably the new __repr__ Updating the notebook is probably long overdue so lets do than here too

@nataliejpg
Copy link
Contributor

i like this :)

@WilliamHPNielsen
Copy link
Contributor

I just pushed a test for cascading. The test fails, but shouldn't it pass? Did I miss how the cascading works?

@astafan8
Copy link
Contributor

astafan8 commented Aug 27, 2018

@WilliamHPNielsen but where have you specified/assigned cfg['core']['station_configurator']['default_folder'] before asserting the value?

@WilliamHPNielsen
Copy link
Contributor

@astafan8, I haven't, that's the whole point. It is specified in the default config file that ships with QCoDeS, qcodes/config/qcodesrc.json. The idea is that the settings in this file are always applied, unless overwritten by another config file. That other config file can thus specify only the few settings that it wants to change.

@WilliamHPNielsen
Copy link
Contributor

But there was a stupid error in the assert statement in my test, so luckily the config system is not broken 🙂

@jenshnielsen jenshnielsen merged commit a06166b into microsoft:master Aug 28, 2018
@jenshnielsen jenshnielsen deleted the config_fixes branch August 28, 2018 12:51
giulioungaretti pushed a commit that referenced this pull request Aug 28, 2018
Merge: 7fe1643 01c7b25
Author: Jens Hedegaard Nielsen <jenshnielsen@gmail.com>

    Merge pull request #1242 from jenshnielsen/config_fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update docs about QCoDeS config (Configuring_QCoDeS.ipynb)
4 participants