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

[DOC] move default from end of argument description to type name #4040

Merged
merged 40 commits into from
Oct 17, 2023
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
94af249
create script to update defaults
Remi-Gau Oct 8, 2023
8353347
[DATALAD RUNCMD] move defaults
Remi-Gau Oct 8, 2023
ad112c0
lint
Remi-Gau Oct 8, 2023
d765818
Apply suggestions from code review
Remi-Gau Oct 8, 2023
670b786
Apply suggestions from code review
Remi-Gau Oct 8, 2023
426ae7e
fix
Remi-Gau Oct 8, 2023
251600e
more fixes
Remi-Gau Oct 8, 2023
0d05d8b
more fixes
Remi-Gau Oct 8, 2023
68d5fa8
move defaults
Remi-Gau Oct 8, 2023
0dcd467
[DATALAD RUNCMD] move defaults
Remi-Gau Oct 8, 2023
4e5ffcd
lint
Remi-Gau Oct 8, 2023
bbd3cd1
update defaults
Remi-Gau Oct 8, 2023
78ddc90
update defaults
Remi-Gau Oct 8, 2023
d968c23
update defaults
Remi-Gau Oct 8, 2023
9a4936e
manual fixes
Remi-Gau Oct 8, 2023
a8d5e02
Update nilearn/plotting/displays/_slicers.py
Remi-Gau Oct 8, 2023
29e95af
fix
Remi-Gau Oct 8, 2023
51356c8
manual fixes
Remi-Gau Oct 8, 2023
367d216
rm md
Remi-Gau Oct 8, 2023
34e8f6d
Update nilearn/reporting/html_report.py
Remi-Gau Oct 8, 2023
e23619e
Update nilearn/reporting/html_report.py
Remi-Gau Oct 8, 2023
e78e8dc
update defatuls
Remi-Gau Oct 9, 2023
f03e5a6
mv defaults
Remi-Gau Oct 9, 2023
c79361e
mv defaults
Remi-Gau Oct 9, 2023
f042d8c
fix
Remi-Gau Oct 9, 2023
6ea296e
add default for data fetcher
Remi-Gau Oct 9, 2023
5f80bc6
Apply suggestions from code review
Remi-Gau Oct 13, 2023
4e6afae
Update nilearn/datasets/func.py
Remi-Gau Oct 13, 2023
c4f9a54
Update nilearn/reporting/glm_reporter.py
Remi-Gau Oct 13, 2023
8749722
Update nilearn/regions/region_extractor.py
Remi-Gau Oct 13, 2023
369dc9e
Update nilearn/plotting/html_surface.py
Remi-Gau Oct 13, 2023
ad9c401
Update nilearn/maskers/multi_nifti_labels_masker.py
Remi-Gau Oct 13, 2023
f1eb948
Merge branch 'main' into default
Remi-Gau Oct 13, 2023
d2e24ae
lint
Remi-Gau Oct 13, 2023
7e44e65
Merge remote-tracking branch 'upstream/main' into default
Remi-Gau Oct 13, 2023
624f3b8
Update nilearn/_utils/data_gen.py
Remi-Gau Oct 13, 2023
86712c1
remove script
Remi-Gau Oct 16, 2023
b520164
adapt with comments from review
Remi-Gau Oct 16, 2023
89c46cc
fix target ref
Remi-Gau Oct 16, 2023
3fb3c69
fix
Remi-Gau Oct 16, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
167 changes: 167 additions & 0 deletions maint_tools/check_defaults.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
"""Update default values in docstrings."""
from __future__ import annotations

import ast
import itertools
from pathlib import Path

from docstring_parser import parse
from docstring_parser.common import DocstringStyle
from rich import print

SKIP_PRIVATE = False


def root_dir() -> Path:
"""Return path to root directory."""
return Path(__file__).parent.parent


def check_docstring(docstring: str, file: Path, lineno: int) -> str:
"""Check docstring content."""
docstring = parse(docstring, style=DocstringStyle.NUMPYDOC)

targets = [
"Default=",
"Default =",
"Default :",
"Default:",
"Default = ",
"default=",
"Default is ",
"Defaults to ",
]
targets += [target.lower() for target in targets]

# for param in docstring.params:
# if param.description is not None and (
# "default " in param.description or
# "Default " in param.description):
# print(
# f" default found '{param.arg_name}' "
# f"in {file.resolve()}:{lineno}"
# )

for target_str, param in itertools.product(targets, docstring.params):
update_docstring(param, target_str, file, lineno)


def update_docstring(param, target_str, file, lineno):
"""Update parameters default in docstring."""
if param.arg_name.startswith("%("):
return

if (
param.description is not None
and target_str in param.description
and param.default is None
):
# extract default value from description
default = param.description.split(target_str)[1].split(".")
default = ".".join(default[:-1])

type_name = f"{param.type_name}, default={default}"

print(
f"updating '{param.arg_name}' in {file.resolve()}:{lineno}",
f"with '{default}'",
)

with open(file) as f:
content = f.readlines()

with open(file, "w") as f:
update_def = False
update_desc = False
# skip the line from beginning of file to lineno
for i, line in enumerate(content):
if i < lineno:
update_def = False
update_desc = False
elif i == lineno:
update_def = True
update_desc = False

if update_def and line.startswith(f" {param.arg_name} :"):
f.write(f" {param.arg_name} : {type_name}\n")
print(" updating type name")
update_def = False
update_desc = True

elif update_def and line.startswith(
f" {param.arg_name} :"
):
f.write(f" {param.arg_name} : {type_name}\n")
print(" updating type name")
update_def = False
update_desc = True

elif update_desc:
if line == f" {target_str}{default}.\n":
f.write("")
print(" updating description")
update_desc = False
elif line.endswith(f" {target_str}{default}.\n"):
f.write(line.replace(f" {target_str}{default}.", ""))
update_desc = False
print(" updating description")
else:
f.write(line)

else:
f.write(line)


def check_functions(body, file):
"""Check functions of a module or methods of a class."""
for node in body:
if isinstance(node, ast.FunctionDef):
if SKIP_PRIVATE and node.name.startswith("_"):
continue
# print(f"function: '{node.name}' in "
# "{file.resolve()}:{node.lineno}")
docstring = ast.get_docstring(node)
docstring = check_docstring(docstring, file, node.lineno)


def main():
"""Update defaults."""
modules = (root_dir() / "nilearn").glob("**/*.py")

files_to_skip = ["test_", "conftest.py"]

for file in modules:
if (
any(file.name.startswith(s) for s in files_to_skip)
or file.parent.name == "tests"
):
continue

if SKIP_PRIVATE and (
file.name.startswith("_") or file.parent.name.startswith("_")
):
continue

with open(file) as f:
module = ast.parse(f.read())

check_functions(module.body, file)

class_definitions = [
node for node in module.body if isinstance(node, ast.ClassDef)
]

for class_def in class_definitions:
# print(
# f"class: '{class_def.name}' "
# f"in {file.resolve()}:{class_def.lineno}"
# )

docstring = ast.get_docstring(class_def)
check_docstring(docstring, file, class_def.lineno)

check_functions(class_def.body, file)


if __name__ == "__main__":
main()
Copy link
Member

Choose a reason for hiding this comment

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

When (and by whom) is this script supposed to be run ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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:

https://github.com/nilearn/nilearn_sandbox/blob/master/nilearn_sandbox/_utils/missing_docstring_default.py

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

move the script to a PR in the sandbox

15 changes: 7 additions & 8 deletions nilearn/_utils/cache_mixin.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ def _check_memory(memory, verbose=0):
Used to cache the masking process.
If a str is given, it is the path to the caching directory.

verbose : int, optional
Verbosity level. Default=0.
verbose : int, default=0
Verbosity level.

Returns
-------
Expand Down Expand Up @@ -117,10 +117,9 @@ def cache(
be cached or not (if user_memory_level is equal of greater than
func_memory_level the function is cached).

shelve : bool, optional
shelve : bool, default=False
Whether to return a joblib MemorizedResult, callable by a .get()
method, instead of the return value of func.
Default=False.

kwargs : keyword arguments, optional
The keyword arguments passed to memory.cache.
Expand Down Expand Up @@ -208,13 +207,13 @@ def _cache(self, func, func_memory_level=1, shelve=False, **kwargs):
func : function
The function the output of which is to be cached.

func_memory_level : int, optional
func_memory_level : int, default=1
The memory_level from which caching must be enabled for the wrapped
function. Default=1.
function.

shelve : bool, optional
shelve : bool, default=False
Whether to return a joblib MemorizedResult, callable by a .get()
method, instead of the return value of func. Default=False.
method, instead of the return value of func.

Returns
-------
Expand Down
7 changes: 3 additions & 4 deletions nilearn/_utils/class_inspect.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,12 @@ def enclosing_scope_name(ensure_estimator=True, stack_level=2):

Parameters
----------
ensure_estimator : boolean, optional
ensure_estimator : boolean, default=True
If true, find the enclosing object deriving from 'BaseEstimator'.
Default=True.

stack_level : integer, optional
stack_level : integer, default=2
If ensure_estimator is not True, stack_level quantifies the
number of frame we will go up. Default=2.
number of frame we will go up.

"""
try:
Expand Down