completing docstring for parameters of get_next_sub #482
Conversation
There was a problem hiding this comment.
Thanks @Transyltooniaa! It's great to see the docstring has now got all of the arguments. Please see a few comments below, related to a new issue that this PR prompted me to open #483 based on a previous comment. This has been very useful for intiating that, thanks!
Because get_next_ses is almost identical to this, once the docstring here is finalised it makes sense to review get_next_ses docstring to match it match as close as possible.
Thanks for this! let me know if you have any questions.
| to find the next subject number. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| top_level_folder : TopLevelFolder | ||
| The top-level folder structure where subject data is organized. |
There was a problem hiding this comment.
This could be The top-level folder, "rawdata" or "derivatives"
| return_with_prefix : bool | ||
| If `True`, return with the "sub-" prefix. | ||
| return_with_prefix : bool, optional | ||
| If `True`, return the subject with the "sub-" prefix. Defaults to `True`. |
There was a problem hiding this comment.
Here further to this comment and (just added) #483, we will remove type hints and defaults from the doctrings. So I think we can revert the changes that the default (but feel free to keep optional and these can all be removed together). thanks!
| `local_path` and `central_path`. If in local-project mode, | ||
| this flag is ignored. | ||
| local_only : bool, optional | ||
| If `True`, only get names from `local_path`; otherwise, |
There was a problem hiding this comment.
Maybe instead of ; otherwise, it can be a new sentence, starting If False`, ...
| local_only : bool, optional | ||
| If `True`, only get names from `local_path`; otherwise, | ||
| retrieve names from both `local_path` and `central_path`. | ||
| If in local-project mode, this flag is ignored. Defaults to `False`. |
There was a problem hiding this comment.
Remove default argument from here, its a nice idea but just because we dont use them elsewhere in the codebase and we will remove type information soon.
|
Hi @Transyltooniaa do you think you will have time to finish this? |
|
Hey @Transyltooniaa I saw your other comment about exams, please ignore my above message. Good luck! |
Before submitting a pull request (PR), please read the contributing guide.
Please fill out as much of this template as you can, but if you have any problems or questions, just leave a comment and we will help out :)
Description
What is this PR
Why is this PR needed?
Addressing Issue #437
What does this PR do?
References
Please reference any existing issues/PRs that relate to this PR.
#437
How has this PR been tested?
Please explain how any new code has been tested, and how you have ensured that no existing functionality has changed.
Tested using running the command
make htmland rendering the new HTML doc generated on the local system and veryifyng if the changes in the function are incorporated in the webpage.Is this a breaking change?
If this PR breaks any existing functionality, please explain how and why.
No
Does this PR require an update to the documentation?
It updates the documentation
If any features have changed, or have been added. Please explain how the
documentation has been updated.
Checklist: