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

bh_print_sys_params: update to python3 and other enhancements. #46

Merged
merged 1 commit into from
Jun 5, 2019
Merged

bh_print_sys_params: update to python3 and other enhancements. #46

merged 1 commit into from
Jun 5, 2019

Conversation

dmopalmer
Copy link

  • Fix issue bh_print_sys_params() fails on python #45 to make bh_print_sys_params work with python3.

  • Accept the direct output of load_set() so that it is optional to extract ['sys_params']

  • Format with %g instead of %f so that typical values (e.g. 1e-11 seconds Time/Chan) are not displayed as zero.

@tritemio tritemio merged commit 90682e7 into Photon-HDF5:master Jun 5, 2019
@tritemio
Copy link
Contributor

tritemio commented Jun 5, 2019

Thanks!

@tritemio
Copy link
Contributor

tritemio commented Jun 6, 2019

@dmopalmer, please let me know what line to use to add you in the contributor's list in the README.

@dmopalmer
Copy link
Author

Contributor:
David Palmer (@dmopalmer)

Adding a useful test would require another change, so that print_sys_params can write to a file-object instead of just to stdout. (This change is worthwhile and simple, but not in the current pull).

@tritemio
Copy link
Contributor

I'll add you to the contributors, thanks. Your original PR is already merged.

For a test, I was simply thinking of a smoke test to test that the function runs with no error, not checking what is printed. A smoke test would have caught this py2 vs py3 error a long time ago.

It is up to you. If you want to add a test I would merge it right away. You could take the chance to convert also the other test (there is only one so far :-() in test_bhreader.py to use the simpler pytest format. Such a contribution would be appreciated.

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