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

API: Make write_brainvision take keyword arguments only #57

Merged
merged 5 commits into from
Nov 7, 2020

Conversation

sappelhoff
Copy link
Member

write_brainvision is a long function with many parameters.

Making these parameters keyword only gives us two advantages:

  • during development we can in the future move around the parameters as much as we want without breaking anybody's code
  • it forces the user to be aware of what argument they pass to which parameter, and errors are less likely

@codecov
Copy link

codecov bot commented Nov 7, 2020

Codecov Report

Merging #57 (65181ad) into master (54da125) will decrease coverage by 2.80%.
The diff coverage is 89.23%.

Impacted file tree graph

@@             Coverage Diff             @@
##            master      #57      +/-   ##
===========================================
- Coverage   100.00%   97.19%   -2.81%     
===========================================
  Files            3        3              
  Lines          264      285      +21     
===========================================
+ Hits           264      277      +13     
- Misses           0        8       +8     
Impacted Files Coverage Δ
pybv/tests/test_bv_writer.py 94.26% <85.71%> (-5.74%) ⬇️
pybv/__init__.py 100.00% <100.00%> (ø)
pybv/io.py 99.37% <100.00%> (-0.63%) ⬇️

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 05f6461...65181ad. Read the comment docs.

@@ -30,7 +30,7 @@
SUPPORTED_UNITS = ['V', 'mV', 'µV', 'uV', 'nV']


def write_brainvision(data, sfreq, ch_names, fname_base, folder_out,
def write_brainvision(*, data, sfreq, ch_names, fname_base, folder_out,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess I would allow for data to be passed by position, but it's a matter of taste I suppose!

Suggested change
def write_brainvision(*, data, sfreq, ch_names, fname_base, folder_out,
def write_brainvision(data, *, sfreq, ch_names, fname_base, folder_out,

Copy link
Member Author

Choose a reason for hiding this comment

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

I would like to keep it all keyword, no positional because I think it leads to a better consistency in this case

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd also prefer the first argument to not be kw-only, but this writer is different enough that I'm also fine with all kw-only (normally the data is the first and the file name the second parameter).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with either, just meant to throw in another possibility :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay great, thanks both! I am merging then and following up with a huge API PR that'll better align what we are doing with the BrainVision spec.

@sappelhoff sappelhoff added this to the 0.4.0 milestone Nov 7, 2020
@sappelhoff sappelhoff merged commit f1aecaa into bids-standard:master Nov 7, 2020
@sappelhoff sappelhoff deleted the positional branch January 27, 2021 16:32
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