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

Reorganization of sqlite_base.py #1570

Merged
merged 37 commits into from
May 24, 2019

Conversation

astafan8
Copy link
Contributor

@astafan8 astafan8 commented May 17, 2019

The reason of this refactoring is to allow database upgraders to have snippets of code from qcodes that are only relevant to those database versions. For example, when we remove layouts and dependencies tables we will still preserve the code that works with them "as is" in a module for the relevant database upgrade, and will change the sqlite API that qcodes uses as we need (this is the reason for Codacy to report problems on this PR).

The new structure is somewhat explained in the top-level docstrings of the new sqlite modules and sub-modules. I also tried to make good commits, so it might be helpful to just follow the commit history to see what happened.

The code has only been moving, and import statements have been adjusted. So, the functionality and bodies of functions are intact.

Note that dataset/sqlite_base.py and dataset/database.py modules are now NOT used inside qcodes, but are left with imported entities to facilitate migration of other dev branches of qcodes, and, unfortunately, some third-party tools. All the new development shall be done in the new sqlite package.

Note imports of two notebooks were updated due to this change.

not gonna do now:

  • !!! apply some privatization scheme so that sqlite stuff does not get used all-over-the-place (to-be-discussed offline)
  • move sqlite3 adapters and converters into their own module(s)
  • go through all of the dataset module and move all sqlite-query code (for example, in experiment container module) to sqlite.*

@QCoDeS/core

qcodes.dataset.database now contains only import statements for
backwards compatibility. in following commits, all QCoDeS will use
sqlite.database instead of qcodes.dataset.database.
and _latest_available_version as well
This makes the __init__ of sqlite.db_upgrades more concise, and also
serves as an example of how to encapsulate code needed by a
particular upgrader function.
sqlite_base is left now empty but with all the imports that it used
to have for backwards compatibility.
Jupyter Notebooks were not adjusted in this commit.
@astafan8 astafan8 marked this pull request as ready for review May 20, 2019 13:06
@codecov
Copy link

codecov bot commented May 20, 2019

Codecov Report

Merging #1570 into master will increase coverage by 0.06%.
The diff coverage is 94.47%.

@@            Coverage Diff             @@
##           master    #1570      +/-   ##
==========================================
+ Coverage   72.12%   72.18%   +0.06%     
==========================================
  Files         105      112       +7     
  Lines       12305    12293      -12     
==========================================
- Hits         8875     8874       -1     
+ Misses       3430     3419      -11

@astafan8
Copy link
Contributor Author

@QCoDeS/core I would like to try to be annoying and propose to take care of apply some privatization scheme so that sqlite stuff does not get used all-over-the-place in a different PR. So, with your review, please tell if you agree with the structure or not (suggestions/questions are welcome).

Copy link
Collaborator

@jenshnielsen jenshnielsen left a comment

Choose a reason for hiding this comment

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

I think the structure is good 👍 My only question is if we should warn on importing from the legacy locations.

qcodes/dataset/database.py Show resolved Hide resolved
qcodes/dataset/sqlite/__init__.py Show resolved Hide resolved
qcodes/dataset/sqlite_base.py Show resolved Hide resolved
@WilliamHPNielsen WilliamHPNielsen merged commit 882ab74 into microsoft:master May 24, 2019
giulioungaretti pushed a commit that referenced this pull request May 24, 2019
Merge: a4aab28 00a68e2
Author: William H.P. Nielsen <whpn@mailbox.org>

    Merge pull request #1570 from astafan8/sqlite-base-reorg
@astafan8 astafan8 deleted the sqlite-base-reorg branch July 22, 2019 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants