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

[pydoclint] Add docstring-missing-parameter and docstring-extraneous-parameter (DOC101, DOC102) #13280

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

augustelalande
Copy link
Contributor

Summary

Add docstring-missing-parameter and docstring-extraneous-parameter (DOC101, DOC102). These rules check that the parameters defined in a functions signature match thos defined in the docstring.

Part of #12434.

Test Plan

Test cases added.

Copy link

codspeed-hq bot commented Sep 8, 2024

CodSpeed Performance Report

Merging #13280 will not alter performance

Comparing augustelalande:doc10x (76c1e5f) with main (a7b8cc0)

Summary

✅ 32 untouched benchmarks

Copy link
Contributor

github-actions bot commented Sep 8, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+8948 -3 violations, +0 -0 fixes in 4 projects; 50 projects unchanged)

apache/airflow (+6568 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ airflow/api/auth/backend/default.py:36:5: DOC101 Parameter `function` missing from the docstring
+ airflow/api/auth/backend/deny_all.py:38:5: DOC101 Parameter `function` missing from the docstring
+ airflow/api/auth/backend/kerberos_auth.py:142:5: DOC101 These parameters are missing from the docstring: `function`, `find_user`
+ airflow/api/auth/backend/kerberos_auth.py:84:5: DOC101 Parameter `app` missing from the docstring
+ airflow/api/auth/backend/session.py:39:5: DOC101 Parameter `function` missing from the docstring
+ airflow/api/common/delete_dag.py:44:5: DOC101 These parameters are missing from the docstring: `dag_id`, `keep_records_in_log`, `session`
+ airflow/api/common/mark_tasks.py:165:5: DOC101 These parameters are missing from the docstring: `dag`, `session`, `state`, `task_ids`, `run_ids`
+ airflow/api/common/mark_tasks.py:186:5: DOC101 These parameters are missing from the docstring: `tasks`, `downstream`, `upstream`
+ airflow/api/common/mark_tasks.py:206:5: DOC101 These parameters are missing from the docstring: `dag`, `execution_date`, `future`, `past`, `session`
+ airflow/api/common/mark_tasks.py:234:5: DOC101 These parameters are missing from the docstring: `dag`, `run_id`, `future`, `past`, `session`
+ airflow/api/common/mark_tasks.py:263:5: DOC101 These parameters are missing from the docstring: `dag_id`, `run_id`, `state`, `session`
+ airflow/api/common/mark_tasks.py:287:5: DOC101 These parameters are missing from the docstring: `dag`, `execution_date`, `run_id`, `commit`, `session`
+ airflow/api/common/mark_tasks.py:341:5: DOC101 These parameters are missing from the docstring: `dag`, `execution_date`, `run_id`, `commit`, `session`
+ airflow/api/common/mark_tasks.py:432:5: DOC101 These parameters are missing from the docstring: `new_state`, `dag`, `execution_date`, `run_id`, `commit`, `session`
+ airflow/api/common/mark_tasks.py:477:5: DOC101 These parameters are missing from the docstring: `dag`, `execution_date`, `run_id`, `commit`, `session`
+ airflow/api/common/mark_tasks.py:501:5: DOC101 These parameters are missing from the docstring: `dag`, `execution_date`, `run_id`, `commit`, `session`
+ airflow/api/common/mark_tasks.py:56:5: DOC101 These parameters are missing from the docstring: `dag`, `infos`, `state`, `run_type`
+ airflow/api/common/mark_tasks.py:99:5: DOC101 These parameters are missing from the docstring: `tasks`, `run_id`, `execution_date`, `upstream`, `downstream`, `future`, `past`, `state`, `commit`, `session`
+ airflow/api/common/trigger_dag.py:122:5: DOC101 These parameters are missing from the docstring: `dag_id`, `triggered_by`, `run_id`, `conf`, `execution_date`, `replace_microseconds`, `session`
+ airflow/api/common/trigger_dag.py:49:5: DOC101 These parameters are missing from the docstring: `dag_id`, `dag_bag`, `triggered_by`, `run_id`, `conf`, `execution_date`, `replace_microseconds`
+ airflow/api_connexion/endpoints/config_endpoint.py:33:5: DOC101 Parameter `conf_dict` missing from the docstring
+ airflow/api_connexion/endpoints/config_endpoint.py:46:5: DOC101 Parameter `config_option` missing from the docstring
+ airflow/api_connexion/endpoints/config_endpoint.py:51:5: DOC101 Parameter `config_section` missing from the docstring
+ airflow/api_connexion/endpoints/config_endpoint.py:59:5: DOC101 Parameter `config` missing from the docstring
+ airflow/api_connexion/endpoints/config_endpoint.py:64:5: DOC101 Parameter `config` missing from the docstring
+ airflow/api_connexion/endpoints/config_endpoint.py:70:5: DOC101 Parameter `section` missing from the docstring
+ airflow/api_connexion/endpoints/connection_endpoint.py:126:5: DOC101 These parameters are missing from the docstring: `connection_id`, `update_mask`, `session`
+ airflow/api_connexion/endpoints/connection_endpoint.py:159:5: DOC101 Parameter `session` missing from the docstring
+ airflow/api_connexion/endpoints/connection_endpoint.py:65:5: DOC101 These parameters are missing from the docstring: `connection_id`, `session`
+ airflow/api_connexion/endpoints/connection_endpoint.py:79:5: DOC101 These parameters are missing from the docstring: `connection_id`, `session`
+ airflow/api_connexion/endpoints/connection_endpoint.py:99:5: DOC101 These parameters are missing from the docstring: `limit`, `offset`, `order_by`, `session`
+ airflow/api_connexion/endpoints/dag_endpoint.py:107:5: DOC101 These parameters are missing from the docstring: `limit`, `offset`, `tags`, `dag_id_pattern`, `only_active`, `paused`, `order_by`, `fields`, `session`
+ airflow/api_connexion/endpoints/dag_endpoint.py:146:5: DOC101 These parameters are missing from the docstring: `dag_id`, `update_mask`, `session`
+ airflow/api_connexion/endpoints/dag_endpoint.py:170:5: DOC101 These parameters are missing from the docstring: `limit`, `session`, `offset`, `only_active`, `tags`, `dag_id_pattern`, `update_mask`
+ airflow/api_connexion/endpoints/dag_endpoint.py:217:5: DOC101 These parameters are missing from the docstring: `dag_id`, `session`
+ airflow/api_connexion/endpoints/dag_endpoint.py:59:5: DOC101 These parameters are missing from the docstring: `dag_id`, `fields`, `session`
... 6532 additional changes omitted for project

apache/superset (+1471 -2 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ RELEASING/changelog.py:107:9: DOC101 Parameter `git_log` missing from the docstring
+ RELEASING/changelog.py:336:5: DOC101 These parameters are missing from the docstring: `ctx`, `previous_version`, `current_version`
+ RELEASING/changelog.py:348:5: DOC101 Parameter `base_parameters` missing from the docstring
+ RELEASING/changelog.py:381:5: DOC101 These parameters are missing from the docstring: `base_parameters`, `csv`, `access_token`, `risk`
+ RELEASING/changelog.py:52:9: DOC101 Parameter `other` missing from the docstring
+ RELEASING/changelog.py:87:9: DOC101 Parameter `pr_number` missing from the docstring
... 1446 additional changes omitted for rule DOC101
+ superset/advanced_data_type/api.py:70:1: DOC102 These documented parameters are not in the function's signature: `-`, `responses`
+ superset/async_events/api.py:51:1: DOC102 These documented parameters are not in the function's signature: `-`, `responses`
+ superset/charts/api.py:680:1: DOC102 These documented parameters are not in the function's signature: `-`, `-`, `responses`
+ superset/charts/data/api.py:282:1: DOC102 These documented parameters are not in the function's signature: `-`, `responses`
... 1463 additional changes omitted for project

bokeh/bokeh (+339 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ examples/advanced/extensions/parallel_plot/parallel_plot.py:15:5: DOC101 These parameters are missing from the docstring: `df`, `color`, `palette`
+ examples/interaction/js_callbacks/js_on_event.py:16:5: DOC101 These parameters are missing from the docstring: `div`, `attributes`
+ examples/models/gauges.py:33:5: DOC101 Parameter `val` missing from the docstring
+ examples/models/trail.py:20:5: DOC101 These parameters are missing from the docstring: `p1`, `p2`
+ examples/output/jupyter/push_notebook/Numba Image Example.ipynb:cell 22:3:5: DOC101 These parameters are missing from the docstring: `img`, `tmp`
+ examples/server/app/events_app.py:17:5: DOC101 These parameters are missing from the docstring: `div`, `attributes`
... 302 additional changes omitted for rule DOC101
+ src/bokeh/core/has_props.py:424:1: DOC102 These documented parameters are not in the function's signature: `json`, `models`, `setter(ClientSession`
+ src/bokeh/core/property/dataspec.py:449:1: DOC102 Documented parameter `name` is not in the function's signature
+ src/bokeh/core/property/descriptors.py:399:1: DOC102 These documented parameters are not in the function's signature: `json`, `models`
+ src/bokeh/core/property/descriptors.py:664:1: DOC102 Documented parameter `new` is not in the function's signature
... 329 additional changes omitted for project

zulip/zulip (+570 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ analytics/lib/fixtures.py:19:5: DOC101 These parameters are missing from the docstring: `days`, `business_hours_base`, `non_business_hours_base`, `growth`, `autocorrelation`, `spikiness`, `holiday_rate`, `frequency`, `partial_sum`, `random_seed`
+ analytics/migrations/0015_clear_duplicate_counts.py:8:5: DOC101 These parameters are missing from the docstring: `apps`, `schema_editor`
+ analytics/tests/test_counts.py:257:9: DOC101 These parameters are missing from the docstring: `table`, `arg_keys`, `arg_values`
+ confirmation/models.py:279:5: DOC101 These parameters are missing from the docstring: `user_profile`, `email_type`
+ confirmation/models.py:298:5: DOC101 Parameter `creation_key` missing from the docstring
+ confirmation/models.py:86:5: DOC101 These parameters are missing from the docstring: `confirmation_key`, `confirmation_types`, `mark_object_used`
... 564 additional changes omitted for rule DOC101
+ zerver/lib/test_helpers.py:781:5: D400 First line should end with a period
- zerver/lib/test_helpers.py:781:5: D400 First line should end with a period
... 563 additional changes omitted for project

Changes by rule (5 rules affected)

code total + violation - violation + fix - fix
DOC101 8895 8895 0 0 0
DOC102 50 50 0 0 0
D205 2 1 1 0 0
D212 2 1 1 0 0
D400 2 1 1 0 0

@MichaReiser MichaReiser added rule Implementing or modifying a lint rule preview Related to preview mode features labels Sep 10, 2024
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

This is great. I haven't gone through the ecossytem results but I think there are a few cases where we don't parse the parameter names correctly.

20 |
21 | / Args:
22 | | lst (list of int): A list of integers.
23 | | multiplier (int): The multiplier for each element in the list.
Copy link
Member

Choose a reason for hiding this comment

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

Highlighting a multiline-range always leads to somewhat awkward code frame rendering. I think it would be better to change the diagnostic range to the multiplier code frame rather than the entire args section. Or is there precedence in other pydocstyle rules to highlight the entire section?

If this range is due to creating a single diagnostic for all the function's extraneous parameters than I think I prefer having multiple diagnostics instead (also makes it easier to suppress an error in case that's needed without risking silencing new extraneous parameters)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is how I have done it previously for example DOC502. The main decision factor is that it's a bit awkward to get the range of the single entry.

Co-authored-by: Micha Reiser <[email protected]>
@MichaReiser
Copy link
Member

Hmm, there's an overlap with https://docs.astral.sh/ruff/rules/undocumented-param/. Not quiet sure how to resolve the overlap.

@augustelalande
Copy link
Contributor Author

UndocumentedParam in its current implimentation is restricted to google-style docstring only. I would suggest deprecating that rule in favor of this one, since it makes more sense as part of the pydoclint package.

@MichaReiser
Copy link
Member

MichaReiser commented Sep 10, 2024

UndocumentedParam in its current implimentation is restricted to google-style docstring only. I would suggest deprecating that rule in favor of this one, since it makes more sense as part of the pydoclint package.

That's fair. But the google style one is less strict than the new one. It doesn't require that you document the parameters of all functions. It only requires that the parameters match the function's parameters if there's an Arguments section.

@augustelalande
Copy link
Contributor Author

augustelalande commented Sep 10, 2024

We could add an option to only highlight violations when a section is present. I tried something similar #13302.

@MichaReiser
Copy link
Member

We plan to look into the conflict but it may take some time. What we could do to move DOC102 forward is to move its implementation out of this PR and try to see if it is possible to share logic with D417:

fn missing_args(checker: &mut Checker, docstring: &Docstring, docstrings_args: &FxHashSet<String>) {
let Some(function) = docstring.definition.as_function_def() else {
return;
};
// Look for arguments that weren't included in the docstring.
let mut missing_arg_names: FxHashSet<String> = FxHashSet::default();
// If this is a non-static method, skip `cls` or `self`.
for ParameterWithDefault {
parameter,
default: _,
range: _,
} in function
.parameters
.iter_non_variadic_params()
.skip(usize::from(
docstring.definition.is_method()
&& !is_staticmethod(&function.decorator_list, checker.semantic()),
))
{
let arg_name = parameter.name.as_str();
if !arg_name.starts_with('_') && !docstrings_args.contains(arg_name) {
missing_arg_names.insert(arg_name.to_string());
}
}
// Check specifically for `vararg` and `kwarg`, which can be prefixed with a
// single or double star, respectively.
if let Some(arg) = function.parameters.vararg.as_ref() {
let arg_name = arg.name.as_str();
let starred_arg_name = format!("*{arg_name}");
if !arg_name.starts_with('_')
&& !docstrings_args.contains(arg_name)
&& !docstrings_args.contains(&starred_arg_name)
{
missing_arg_names.insert(starred_arg_name);
}
}
if let Some(arg) = function.parameters.kwarg.as_ref() {
let arg_name = arg.name.as_str();
let starred_arg_name = format!("**{arg_name}");
if !arg_name.starts_with('_')
&& !docstrings_args.contains(arg_name)
&& !docstrings_args.contains(&starred_arg_name)
{
missing_arg_names.insert(starred_arg_name);
}
}
if !missing_arg_names.is_empty() {
if let Some(definition) = docstring.definition.name() {
let names = missing_arg_names.into_iter().sorted().collect();
checker.diagnostics.push(Diagnostic::new(
UndocumentedParam {
definition: definition.to_string(),
names,
},
function.identifier(),
));
}
}
}

@charliermarsh
Copy link
Member

I agree that the rule makes more sense here semantically (since it's about content rather than style)... though the D rules are a lot more popular so we'd lose something by moving the rule over. I find this one to be a really tough call (and it's making me desperate for re-categorized rulesets with a single coherent docstring section).

Anyway... I think I'm comfortable adding this to DOC in preview, then eventually redirecting D417 to it. I'd like to get @zanieb's input on this one though.

@zanieb
Copy link
Member

zanieb commented Sep 24, 2024

@charliermarsh that sounds okay to me

@augustelalande
Copy link
Contributor Author

Thanks guys. Is this ready for merge, or are there more changes requested?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants