Refactor phys2bids interfaces into single script io.py

[vinferrer]

Closes #295

Merges txt.pyand acq.py into io.py

Proposed Changes

• Move the relevant functions to the same submodule (phys2bids.io). If you're concerned about their respective imports, you can just move the tool-specific imports into the function definitions.
• Rename the functions from populate_phys_info() to load_acq_ext(), and load_text_ext() (or similar).
• Update tests and API documentation.

Change Type

• bugfix (+0.0.1)
• minor (+0.1.0)
• major (+1.0.0)
• internal (no version update)
• documentation (no version update)

Checklist before review

• I added everything I wanted to add to this PR.
• [Code or tests only] I wrote/updated the necessary docstrings.
• [Code or tests only] I ran and passed tests locally.
• [Documentation only] I built the docs locally.
• My contribution is harmonious with the rest of the code: I'm not introducing repetitions.
• My code respects the adopted style, especially linting conventions.
• The title of this PR is explanatory on its own, enough to be understood as part of a changelog.
• I added or indicated the right labels.

• I added information regarding the timeline of completion for this PR.

• Please, comment on my PR while it's a draft and give me feedback on the development!

[codecov]

Codecov Report

Merging #344 (b956c17) into master (b965b03) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #344      +/-   ##
==========================================
+ Coverage   94.89%   94.93%   +0.03%     
==========================================
  Files           9        8       -1     
  Lines         843      868      +25     
==========================================
+ Hits          800      824      +24     
- Misses         43       44       +1     

Impacted Files	Coverage Δ
phys2bids/io.py	98.80% <100.00%> (ø)
phys2bids/phys2bids.py	89.59% <100.00%> (+0.06%)	⬆️
phys2bids/utils.py	97.97% <0.00%> (-0.62%)	⬇️

[eurunuela]

Thank you @vinferrer ! I don't have the bandwidth right now to review this but I will.

I see you selected both minormod and majormod. How's that?

Also, we would be going under 90% code coverage, so we'll need some tests.

[vinferrer]

Thank you @vinferrer ! I don't have the bandwidth right now to review this but I will.

I see you selected both minormod and majormod. How's that?

I don't know what to do about it.

Also, we would be going under 90% code coverage, so we'll need some tests.

That's weird i didn't change any tests i just moved the functions around and modified the imports in the testing

[vinferrer]

I forgot to commit the testing function, 😆

[eurunuela]

Now that I think of it, I believe this is an internal update. Not a majormod or a minormod. It really doesn't change the output of the package but makes our lives easier.

What do you think @tsalo @smoia ?

[smoia]

Hey y'all! Sorry, I noticed I didn't add the refactoring to the possible change types. But @vinferrer labelled it right: it's a refactoring.

"Internal" is for the infrastructure. Maybe we can change the label, now that I think about it...

[smoia]

BTW: I would get rid of the "interfaces" folder and rename io.py into interfaces.py. (It's not really an output function, is it?)

[tsalo]

An interface implies a class to me, with bidirectional interaction with the source. Having an input/output module also lets you put the write_json, write_file, etc. functions in the same place.

[eurunuela]

I like io.py better too.

[vinferrer]

so should i get rid of the folder then ?

[eurunuela]

so should i get rid of the folder then ?

I would say yes. It does not make much sense to have the folder with just one file. And usually, io.py goes on the same level as utils.py for instance.

[vinferrer]

okay

[vinferrer]

I think this is ready for review then

[smoia]

Generally LGTM, but unless everyone else is ok with it, could you remove _ext from the name of the functions?

[vinferrer]

Generally LGTM, but unless everyone else is ok with it, could you remove _ext from the name of the functions?

Actually no, this is something that i put to remark that we are processing extensions .txt is an extension, then we have formats like acq that can be in .txt or in .acq extensions

[vinferrer]

This is actually related to the next PR I talk about in #295. Because my goal is to have extension functions that will call in the end to the same process fucntion. This way we will reduce the amount of code

[smoia]

Alright, sounds good!
It's ready!

[eurunuela]

LGTM! I just have a tiny comment.

[smoia]

@vinferrer , you merged a PR without being the main reviewer (not a big deal), but also without passing the style check.
Please open a PR now to pass it before anyone else opens a new PR.

[smoia]

🚀 PR was released in 2.3.0 🚀