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

Refactor interfaces/txt.py use of chtrig channel #273

Closed
4 tasks
smoia opened this issue Jul 7, 2020 · 3 comments · Fixed by #305
Closed
4 tasks

Refactor interfaces/txt.py use of chtrig channel #273

smoia opened this issue Jul 7, 2020 · 3 comments · Fixed by #305
Labels
Good first issue Good for newcomers Refactoring Improve nonfunctional attributes

Comments

@smoia
Copy link
Member

smoia commented Jul 7, 2020

After the discussion in #272 , I understand how the current use of chtrig in interfaces/txt.py can lead to confusion.
I'd propose a very quick, very neat refactoring to lower the chances of confusion while reading the code

Detailed Description

In this line and this line, interfaces/txt.py initialise a BlueprintInput object with chtrig +1 as trigger channel.
This could be thought coming from the idea that the first channel becomes time, however it's due to the fact that upstream in the process, here, chtrig is decremented by 1. This could lead to confusion in understanding the script, especially since chtrig is not used in any other place than the BlueprintInput initialisation.

Possible Implementation

  • Delete line 322
  • Remove chtrig from this definition's argoments, since it's not used in the definition itself.
  • Change chtrig +1 in this line and in this line into chtrig only.
  • Add explanation of chtrig indexing starting from 1 for human readability in all the docstrings.

I'm going to add a Good First Issue label since it should be not that hard. @RayStick you're still the guardian of G1Fs.

@smoia smoia added Good first issue Good for newcomers Refactoring Improve nonfunctional attributes labels Jul 7, 2020
@smoia smoia added this to the phys2bids second semester 2020 milestone Jul 30, 2020
@vinferrer
Copy link
Collaborator

Did we stablish a limit on how much time a good first issue can be unresolved?

@vinferrer
Copy link
Collaborator

Since This issue is affecting the implementation of #257 we should move to implement this. If someone with less experience in scripting the toolbox wants to try good. If not I am happy to do this quickly

@eurunuela
Copy link
Collaborator

I'd say you could start to work on it, it's already been two months.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good first issue Good for newcomers Refactoring Improve nonfunctional attributes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants