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

Fix a different in interface between NTParameterSet and the other ParameterSet classes #372

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

apdavison
Copy link
Contributor

NTParameterSet inherits from dict, and so ps.values is a function. In the other classes, ps.values was a dictionary. Here I've renamed ps.values to ps._values, and implemented ps.items() to match the behaviour of dict.

@apdavison apdavison added this to the 0.8 milestone Sep 11, 2017
…ameterSet classes.

NTParameterSet inherits from dict, and so `ps.values` is a function. In the other classes, `ps.values` was a dictionary. Here I've renamed `ps.values` to `ps._values`, and implemented `ps.items()` to match the behaviour of dict.
@coveralls
Copy link

coveralls commented Sep 11, 2017

Coverage Status

Coverage decreased (-0.01%) to 71.519% when pulling c930d3c on apdavison:fix-parameters into d5cc713 on open-research:master.

@apdavison apdavison requested a review from babsey September 11, 2017 17:33
@babsey
Copy link
Contributor

babsey commented Sep 12, 2017

I reviewed this PR and it is a good idea to rename ps.values to ps._values. But ps.items might be not necessary, because ps.as_dict appears a similar method to ps.items.

Copy link
Contributor

@babsey babsey left a comment

Choose a reason for hiding this comment

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

The comment of the method is not changed to self._values (see line 83)

@@ -108,6 +108,9 @@ def parse_command_line_parameter(self, p):
def diff(self, other):
return _dict_diff(self, other)

def items(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

similar to as_dict method

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