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

Instrument.close #285

Merged
merged 7 commits into from
Jul 27, 2016
Merged

Instrument.close #285

merged 7 commits into from
Jul 27, 2016

Conversation

eendebakpt
Copy link
Contributor

Fixes #277

Changes proposed in this pull request:

  • Do not strip name from instrument when closing
  • Add tests

@alexcjohnson @giulioungaretti

get_cmd=partial(self.get_gate, g),
set_cmd=partial(self.set_gate, g),
get_parser=float,
vals=Numbers(-800, 400))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is normally what ManualParameter is for, like in the ithaco driver

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the ManualParameter, makes the code look cleaner. The ManualParameter did not accept the get_parser argument. Is there a reason for this? I would like to be able to do a get_parser=float to make sure my value is a float.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should only allow you to store (and thus retrieve) values consistent with its Validator... Is that not enough to ensure you get the type you want?

@@ -322,7 +328,7 @@ def __dir__(self):
return sorted(set(names))


def strip_attrs(obj):
def strip_attrs(obj, whitelist=[]):
Copy link
Contributor

Choose a reason for hiding this comment

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

For safety, this should be immutable whitelist=() or even

def strip_attrs(obj, whitelist=None):
    if whitelist is None:
        whitelist = []

Copy link
Contributor

Choose a reason for hiding this comment

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

also whitelist needs to go in the docstring

Copy link
Contributor Author

@eendebakpt eendebakpt Jul 27, 2016

Choose a reason for hiding this comment

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

@alexcjohnson
Are you worried that someone might add something like

def strip_attrs(obj, whitelist=[]):
   ...
   whitelist += ['dummy']
    ...

?

In that case I think whitelist=() is also insufficient. The version with None is sufficient, but it makes the code longer. I would prefer the C/C++ style const argument to indicate that an argument will not be modified.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just bad practice to use mutable default args (unless you actually intend that behavior, which is rather unusual - about only case I'm aware of is for caching, and you need to be quite careful with it) http://docs.python-guide.org/en/latest/writing/gotchas/

Nothing will break right now but you just have to imagine someone going in and mutating the whitelist ("oh, we never want to strip a name attribute so I'll just add it here: whitelist.append('name')") and then things start going crazy after a while.

@alexcjohnson
Copy link
Contributor

@eendebakpt after the comments I just made about strip_attrs, this is ready to 💃 - good tests, thanks!

@eendebakpt
Copy link
Contributor Author

Updated the docstrings and made the whitelist argument a tuple (), if no more comments then I will merge

@alexcjohnson
Copy link
Contributor

perfect 💃

@giulioungaretti
Copy link
Contributor

actually, docstrings have to use double triple quotes :D

@eendebakpt eendebakpt merged commit d8e381f into master Jul 27, 2016
@peendebak peendebak deleted the instrument/close branch August 3, 2016 20:30
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.

3 participants