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

make print_readable_snapshot work with value-less parameters #844

Conversation

WilliamHPNielsen
Copy link
Contributor

If a parameter does not have a value in the snapshot (e.g. because it is an ArrayParameter), the print_readable_snapshot function can not iterate through all parameters and values. This PR fixes that.

Changes proposed in this pull request:

  • Make print_readable_snapshot handle missing values

@jenshnielsen @AdriaanRol

@@ -227,7 +227,14 @@ def print_readable_snapshot(self, update=False, max_chars=80):
for par in sorted(snapshot['parameters']):
name = snapshot['parameters'][par]['name']
msg = '{0:<{1}}:'.format(name, par_field_len)
val = snapshot['parameters'][par]['value']

# in case of e.g. ArrayParameters, the parameter may not have
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would change the docstring to say in case snapshot_value is False otherwise 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps a slight preference for snapshot['parameters'][par].get('value', 'Not available') in stead of try except

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 wholeheartedly agree with your viewpoint.

@WilliamHPNielsen WilliamHPNielsen merged commit 67826b5 into microsoft:master Nov 2, 2017
giulioungaretti pushed a commit that referenced this pull request Nov 2, 2017
Author: William H.P. Nielsen <[email protected]>

    make print_readable_snapshot work with value-less parameters (#844)
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.

2 participants