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

Consistent import from top-level and first-level namespace of qcodes #1962

Merged
merged 28 commits into from Mar 27, 2020

Conversation

lakhotiaharshit
Copy link
Contributor

This pr removes does not import the legacy api in the top-level namespace of qcodes by default. If the flag for importing legacy api is set to true in the config file then only the legacy-imports are done. It fixes the example notebooks to reflect this change.

It also copies the imports to the init.py files of the respective modules.

@jenshnielsen

@codecov
Copy link

codecov bot commented Mar 24, 2020

Codecov Report

Merging #1962 into master will increase coverage by 0.00%.
The diff coverage is 87.50%.

@@           Coverage Diff           @@
##           master    #1962   +/-   ##
=======================================
  Coverage   69.00%   69.01%           
=======================================
  Files         155      155           
  Lines       19906    19908    +2     
=======================================
+ Hits        13737    13739    +2     
  Misses       6169     6169           

@jenshnielsen
Copy link
Collaborator

Looks good. Other than the change mentioned above I think this just need a changelog entry.

@jenshnielsen
Copy link
Collaborator

jenshnielsen commented Mar 25, 2020

The travis failure probably points to a circular import. We may have to rething the importing slightly. I would probably try to break this by making the imports in serialization relative

@jenshnielsen
Copy link
Collaborator

@lakhotiaharshit I think we just need to fix the 2 long lines then this should be ready to go

@lakhotiaharshit
Copy link
Contributor Author

@lakhotiaharshit I think we just need to fix the 2 long lines then this should be ready to go

@jenshnielsen '2 long lines' as in changelog? I just did that.

Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work! just one question - shouldn't all sub-module-level imports be relative? (see other comments)

from qcodes.data.format import Formatter
from qcodes.data.gnuplot_format import GNUPlotFormat
from qcodes.data.hdf5_format import HDF5Format
from qcodes.data.io import DiskIO
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't these be relative imports in order to make the "module" independent on where it itself is located?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

qcodes/dataset/__init__.py Outdated Show resolved Hide resolved
qcodes/instrument/__init__.py Outdated Show resolved Hide resolved
@lakhotiaharshit
Copy link
Contributor Author

great work! just one question - shouldn't all sub-module-level imports be relative? (see other comments)

Yes, now that you point it out. I think they all should be relative.

@jenshnielsen
Copy link
Collaborator

@lakhotiaharshit as in the feedback from codacy :)

@jenshnielsen jenshnielsen merged commit c8bc7ce into microsoft:master Mar 27, 2020
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.

None yet

3 participants