[DOC] move default from end of argument description to type name #4040
Conversation
=== Do not change lines below ===
{
"chain": [],
"cmd": "python /home/remi/github/nilearn/maint_tools/check_defaults.py",
"exit": 0,
"extra_inputs": [],
"inputs": [],
"outputs": [],
"pwd": "."
}
^^^ Do not change lines above ^^^
|
👋 @Remi-Gau Thanks for creating a PR! Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft. Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.
For new features:
For bug fixes:
We will review it as quick as possible, feel free to ping us with questions if needed. |
Codecov Report
@@ Coverage Diff @@
## main #4040 +/- ##
==========================================
+ Coverage 91.48% 91.50% +0.02%
==========================================
Files 143 143
Lines 16076 16072 -4
Branches 3340 3339 -1
==========================================
Hits 14707 14707
+ Misses 823 820 -3
+ Partials 546 545 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -262,7 +262,9 @@ def custom_function(vertices): | |||
| ] = """ | |||
| data_dir : :obj:`pathlib.Path` or :obj:`str`, optional | |||
There was a problem hiding this comment.
shouldn't we change optional to default=None here?
There was a problem hiding this comment.
That is one thing I think we should probably discuss a bit because in many cases if the default is None then in many cases the code itself will set it to something else if it is None.
In this case if None is passed then data will be installed in the home folder tof the user.
There are many other cases like this in the codebase.
For the moment in this PR I have tried to mostly just move the default that was already in the description to the end of the "type" section.
But it may be good to sort of decide if this default should just be what is in the function declaration or if it should try to show what the "effective" default will be in the cases when None is passed.
I have a slight preference for the latter because it makes the doc a bit more informative and does not just repeat the function définition.
There was a problem hiding this comment.
Good point, we can leave for now.
Though this also applies when an option is e.g. "auto" and it could be the case that the "effective" default of a parameter being left as None changes according to other parameters. But this is or should be addressed rather in the parameter description.
There was a problem hiding this comment.
it is not always the home folder because this can be controlled with environment variables as well. so I think it might be a bit long to explain on the first line of the docstring entry
There was a problem hiding this comment.
True in that case but may be valid for other case where the default is None
Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>
Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>
Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>
Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>
Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>
Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>
maint_tools/check_defaults.py
Outdated
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| main() |
There was a problem hiding this comment.
When (and by whom) is this script supposed to be run ?
There was a problem hiding this comment.
Originally I though that this was a script it would be convenient to "keep around" for maintainers to run "once in a while" to ensure that defaults are declared properly.
But after looking around it feels like the one from @mathdugre in the sandbox:
may be adapted to do a better and more systematic job than I put together here for moving defaults.
This should also be discussed in the context of @ymzayek work to mkae type annotation play nice with writing and rendering of the docstrings in #4049
To keep things "simple" I would suggest:
- move this script to the sandbox for now
- modify @mathdugre scripts to make sure that all functions / methods with default parameters have those defaults described in the docstring (at least for the public API)
- cross the bridge about docstring + type annotation when we get to it.
There was a problem hiding this comment.
move the script to a PR in the sandbox
Changes proposed in this pull request: