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

Add reasonable defaults #133

Merged
merged 4 commits into from
Sep 25, 2019
Merged

Add reasonable defaults #133

merged 4 commits into from
Sep 25, 2019

Conversation

ajtritt
Copy link
Contributor

@ajtritt ajtritt commented Aug 21, 2019

Motivation

Remove boilerplate requirements.

How to test the behavior?

run test suite

Checklist

  • Have you checked our Contributing document?
  • Have you ensured the PR description clearly describes problem and the solution?
  • Is your contribution compliant with our coding style ? This can be checked running flake8 from the source directory.
  • Have you checked to ensure that there aren't other open Pull Requests for the same change?
  • Have you included the relevant issue number using #XXX notation where XXX is the issue number ? By including "Fix #XXX" you allow GitHub to close the corresponding issue.

@ajtritt ajtritt requested a review from oruebel August 21, 2019 18:47
@@ -30,7 +30,7 @@
class HDF5IO(HDMFIO):

@docval({'name': 'path', 'type': str, 'doc': 'the path to the HDF5 file'},
{'name': 'manager', 'type': BuildManager, 'doc': 'the BuildManager to use for I/O', 'default': None},
{'name': 'manager', 'type': (TypeMap, BuildManager), 'doc': 'the BuildManager to use for I/O', 'default': None},
Copy link
Contributor

Choose a reason for hiding this comment

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

The doc should be updated to reflect the new behavior. Should the name of the arg also change?

Copy link
Contributor Author

@ajtritt ajtritt Aug 21, 2019

Choose a reason for hiding this comment

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

The name should stay, for backwards compatibility. I will update the doc though

oruebel
oruebel previously approved these changes Aug 21, 2019
@codecov
Copy link

codecov bot commented Sep 25, 2019

Codecov Report

Merging #133 into dev will decrease coverage by 0.05%.
The diff coverage is 20%.

Impacted file tree graph

@@           Coverage Diff            @@
##              dev   #133      +/-   ##
========================================
- Coverage   69.05%    69%   -0.06%     
========================================
  Files          24     24              
  Lines        4967   4971       +4     
  Branches     1137   1139       +2     
========================================
  Hits         3430   3430              
- Misses       1170   1172       +2     
- Partials      367    369       +2
Impacted Files Coverage Δ
src/hdmf/backends/hdf5/h5tools.py 63.41% <0%> (-0.2%) ⬇️
src/hdmf/build/map.py 59.83% <33.33%> (-0.12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a9a9cb...67d96bf. Read the comment docs.

@ajtritt ajtritt merged commit 5e6a2f6 into dev Sep 25, 2019
@rly rly deleted the enh/default_nsc branch October 16, 2019 00: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.

2 participants