Remove dataset add_parameter #1547
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WilliamHPNielsen
changed the title
WilliamHPNielsen
force-pushed
the
remove/dataset_add_parameter
branch
from
c8eeb5d to
9ce3c16
Compare
Codecov Report
@@ Coverage Diff @@
## master #1547 +/- ##
==========================================
- Coverage 71.56% 71.54% -0.03%
==========================================
Files 105 105
Lines 12141 12137 -4
==========================================
- Hits 8689 8683 -6
- Misses 3452 3454 +2 |
|
@QCoDeS/core, bump, bump. All the commits here are contained in #1555, but it might be good to get this in first. We can also do both in one go. What do you think? |
|
Let's do these PRs one-by-one. I'll have a look at this tomorrow. |
astafan8
approved these changes
May 9, 2019
| raise ValueError(f'Duplicate parameter name: {spec.name}') | ||
|
|
||
| self._interdeps = self._interdeps._extend_with_paramspec(spec) | ||
| raise NotImplementedError('This method has been removed. ' |
There was a problem hiding this comment.
please add a test for this :)
|
I am not super keen on removing code that is public api without a deprecation cycle but I don't have a good solution. Should we check that this is not called from any code that we know of before doing this? |
|
Code that i know (and i checked) of does not use DataSet.add_parameter. |
|
No occurrences in main qcodes (obviously) nor in the current master of |
giulioungaretti
pushed a commit
that referenced
this pull request
May 13, 2019
Merge: ce97b7d 0d59177 Author: William H.P. Nielsen <[email protected]> Merge pull request #1547 from WilliamHPNielsen/remove/dataset_add_parameter
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With the merge of #1477, the method
add_parameterof theDataSetbecame obsolete. This PR removes the method from theDataSetAPI by breaking it. Removing that method will in turn make development of the interdependencies stuff easier, since the methodextend_with_paramspecwill no longer have to exist.If you wish, you can view this PR as the first "clean up the fallout of #1477" PR. There are more to come, including schema upgrades etc.
Still pending: upgrade all tests to useset_interdependenciesinstead ofadd_parameter.I am open to input on how to best break/remove/deprecate
DataSet.add_parameter, but I stress the point that its existence is a bit of a roadblock and that I therefore find it justified to simply kill it without going through a deprecation cycle. Looking ahead, we will need to break a lot of stuff and I think we should allow ourselves to move quickly.@QCoDeS/core this is now ready for review.