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

GroupParameter and Group (for visa commands that set/get more than one parameter at once) #1232

Merged
merged 5 commits into from
Aug 17, 2018

Conversation

sohailc
Copy link
Member

@sohailc sohailc commented Aug 16, 2018

I have created a separate PR for the GroupParameter and added a basic test

@astafan8 astafan8 changed the title first commit GroupParameter and Group (for visa commands that set/get more than one parameter at once) Aug 16, 2018
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.

Sweet! thank you!

@codecov
Copy link

codecov bot commented Aug 16, 2018

Codecov Report

Merging #1232 into master will increase coverage by 0.08%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master    #1232      +/-   ##
==========================================
+ Coverage   80.77%   80.85%   +0.08%     
==========================================
  Files          49       50       +1     
  Lines        6841     6886      +45     
==========================================
+ Hits         5526     5568      +42     
- Misses       1315     1318       +3

def __init__(self, name: str, instrument: Instrument, **kwargs) -> None:
self.group: Union[Group, None] = None
super().__init__(name, instrument=instrument, **kwargs)

def set_raw(self, value: Any):
self.group.set(self, value)
self.set_raw = lambda value: self.group.set(self, value)
Copy link
Contributor

Choose a reason for hiding this comment

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

as we've discussed, i'm ok with reducing the code quality for the sake of keeping this code more readable. It's just the design of Parameter and _BaseParameter does not allow a clean way of redefining set_raw/get_raw. Once the design is fixed, the original implementation of GroupParameter will be correct (and most readable). Hence, I recommend to revert this commit. @jenshnielsen @WilliamHPNielsen

@sohailc sohailc requested review from WilliamHPNielsen and removed request for Dominik-Vogel August 17, 2018 09:39
@sohailc sohailc merged commit c044524 into master Aug 17, 2018
giulioungaretti pushed a commit that referenced this pull request Aug 17, 2018
Merge: 1470658 b43ae7a
Author: sohail chatoor <[email protected]>

    Merge pull request #1232 from QCoDeS/feature/GroupParameter
@Dominik-Vogel
Copy link
Contributor

Dominik-Vogel commented Aug 21, 2018

@QCoDeS/core: This PR is a copy and paste from PR #972 (mostly this commit: b5ddabf).

It is good practice to use git with its features to enable code traceability. Also, as the author of the original code, I feel uncomfortable with a 'copy and pasting' of my code without attribution.

@astafan8 astafan8 deleted the feature/GroupParameter branch October 19, 2018 14:04
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