Overhaul docs, add `pydoclint` and custom formatter #1314

haakonvt · 2023-08-12T04:36:21Z

Description

Step 1: Add docstring linter
Step 2: Fix 1 million errors

Note

It seems the linter struggles with | (Union) in return annotations. I have raised an issue and is waiting for a response. jsh9/pydoclint#66

--> The maintainer fixed it 😄

Also awaiting an answer for a different fix than excluding a file through pre-commit for the with_ parameter for Query:
jsh9/pydoclint#73

--> The maintainer fixed this one as well ❤️

Last bug in pydoclint, currently awaiting response: jsh9/pydoclint#75

--> ...and fixed 🚀

Checklist:

Tests added/updated.
Documentation updated. Documentation is generated from docstrings - these must be updated according to your change.
If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.
Changelog updated in CHANGELOG.md.
Version bumped. If triggering a new release is desired, bump the version number in _version.py and pyproject.toml per semantic versioning.

cognite/client/_api/annotations.py

pyproject.toml

haakonvt · 2023-08-17T21:34:34Z

Started out with 868 issues on master:

Counter({'DOC105': 267,
         'DOC203': 166,
         'DOC103': 93,
         'DOC101': 70,
         'DOC110': 54,
         'DOC501': 50,
         'DOC109': 44,
         'DOC201': 39,
         'DOC403': 28,
         'DOC104': 28,
         'DOC001': 11,
         'DOC102': 9,
         'DOC502': 3,
         'DOC301': 3,
         'DOC402': 2,
         'DOC401': 1})

...now down to 139:

Counter({'DOC501': 53,
         'DOC203': 25,
         'DOC201': 15,
         'DOC105': 9,
         'DOC109': 7,
         'DOC110': 6,
         'DOC103': 5,
         'DOC001': 4,
         'DOC101': 4,
         'DOC502': 3,
         'DOC301': 3,
         'DOC104': 2,
         'DOC403': 1,
         'DOC401': 1,
         'DOC402': 1}

...and finally, 0 🥳

codecov · 2023-08-21T11:05:42Z

Codecov Report

Merging #1314 (0beb8fa) into master (c0c8942) will decrease coverage by 0.09%.
The diff coverage is 98.68%.

@@            Coverage Diff             @@
##           master    #1314      +/-   ##
==========================================
- Coverage   90.42%   90.34%   -0.09%     
==========================================
  Files         110      110              
  Lines       13515    13514       -1     
==========================================
- Hits        12221    12209      -12     
- Misses       1294     1305      +11

Files Changed	Coverage Δ
...ient/_api/data_modeling/_data_modeling_executor.py	`81.25% <ø> (ø)`
cognite/client/_api/transformations/schema.py	`100.00% <ø> (ø)`
cognite/client/data_classes/relationships.py	`95.04% <ø> (ø)`
cognite/client/data_classes/sequences.py	`90.29% <ø> (ø)`
cognite/client/data_classes/shared.py	`94.91% <ø> (ø)`
cognite/client/data_classes/templates.py	`84.79% <ø> (ø)`
cognite/client/data_classes/three_d.py	`89.18% <ø> (ø)`
cognite/client/data_classes/time_series.py	`90.85% <ø> (ø)`
...te/client/data_classes/transformations/__init__.py	`88.35% <ø> (ø)`
...nite/client/data_classes/transformations/common.py	`93.02% <ø> (ø)`
... and 87 more

haakonvt · 2023-08-21T17:22:56Z

Should I bump version? Perhaps "massive" doc fixes is okay? 🤷

doctrino

Only a few minor comments.

doctrino · 2023-08-22T13:11:34Z

cognite/client/_api/datapoint_tasks.py

@@ -739,11 +744,18 @@ def store_partial_result(self, res: DataPointListItem) -> Optional[List[Splittin
            return self._split_self_into_new_subtasks_if_needed(last_ts)
        return None

-    def _create_subtasks_idxs(self, n_new_tasks: int) -> Iterable[Tuple[float, ...]]:
+    def _create_subtasks_idxs(self, n_new_tasks: int) -> Generator[Tuple[float, ...], None, None]:


I think the type hint for generators is not very good, thus I think the old one should stay, if it is possible configure the linter to do so.

Note I am not alone https://stackoverflow.com/questions/42531143/how-to-type-hint-a-generator-in-python-3

I used to agree with this, but was convinced otherwise by the maintainer of pydoclint: jsh9/pydoclint#68 (comment)

cognite/client/_api/datapoints_subscriptions.py

cognite/client/data_classes/data_modeling/instances.py

cognite/client/data_classes/datapoints_subscriptions.py

cognite/client/testing.py

scripts/custom_checks/docstrings.py

erlendvollset

MASSIVE work 💪Docs just leveled up 💯

erlendvollset · 2023-08-22T20:32:05Z

pyproject.toml

@@ -14,6 +14,18 @@ packages = [{ include="cognite", from="." }]
 [tool.ruff.pyupgrade]
 keep-runtime-typing = true

+[tool.pydoclint]


Can we configure it using cmd line args in the pre-commit file instead? We prefer putting it there, only in here if that doesn't work.

Absolutely 👌

erlendvollset

Some nitpicking

erlendvollset · 2023-08-24T10:27:40Z

cognite/client/_api_client.py

@@ -68,24 +64,25 @@

 class APIClient:
    _RESOURCE_PATH: str
+    _RETRYABLE_POST_ENDPOINT_REGEX_PATTERNS = frozenset(


This type of change is not well suited in a 6K line PR of formatting. cognite-sdk-experimental depends on adding regexes to this set, so we're potentially breaking them here depending on how exactly it's done.

Yes, I agree it should probably be put in a separate PR; was a new version of ruff that started complaining about this not being a classvar (as set is ofc mutable).

I see that a "similar" issue has happened before 😉 https://github.com/cognitedata/cognite-sdk-python-experimental/pull/351/files

Conclusion: This change does not affect the experimental SDK, as it is doing s1 |= s2, which is just s1 = s1 + s2, i.e. not an in-place operation. The experimental SDK thus adopts this correction to frozenset without side effects (I've checked all releases back to the "happening" linked above).

I reverted the change and will move it to a separate PR 👍

cognite/client/_api/raw.py

erlendvollset · 2023-08-24T11:50:25Z

cognite/client/_api/datapoints_subscriptions.py

@@ -185,7 +178,7 @@ def iterate_data(
        external_id: str,
        start: str | None = None,
        limit: int = DATAPOINT_SUBSCRIPTION_DATA_LIST_LIMIT_DEFAULT,
-    ) -> Iterator[DatapointSubscriptionBatch]:
+    ) -> Generator[DatapointSubscriptionBatch, None, None]:


Can stay an Iterator

I agree, but then we either need to drop pydoclint (you can't convince the maintainer on this one, unfortunately: https://jsh9.github.io/pydoclint/notes_generator_vs_iterator.html) or ignore all checks on return types. I think just using Generator is the easy solution here.

This reverts commit 600466e.

Co-authored-by: Anders Albert <[email protected]>

haakonvt mentioned this pull request Aug 12, 2023

Fix type hints #1313

Merged

haakonvt force-pushed the add-pydoclint branch 3 times, most recently from 1ef438b to b03fb56 Compare August 14, 2023 19:29

haakonvt commented Aug 15, 2023

View reviewed changes

cognite/client/_api/annotations.py Outdated Show resolved Hide resolved

haakonvt force-pushed the add-pydoclint branch from b03fb56 to b499193 Compare August 15, 2023 12:40

haakonvt commented Aug 15, 2023

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

haakonvt force-pushed the add-pydoclint branch 2 times, most recently from 9eec8c6 to 27f4564 Compare August 16, 2023 02:24

haakonvt mentioned this pull request Aug 17, 2023

Add meta-test to ensure all .py files follow PEP 563 #1318

Merged

4 tasks

haakonvt marked this pull request as ready for review August 17, 2023 21:22

haakonvt requested review from a team as code owners August 17, 2023 21:22

haakonvt force-pushed the add-pydoclint branch from e461a08 to ddd2c48 Compare August 18, 2023 11:58

haakonvt mentioned this pull request Aug 19, 2023

Change test TestHelpers.test_is_retryable_add to avoid mutating class variable #1320

Merged

1 task

haakonvt force-pushed the add-pydoclint branch 2 times, most recently from 0b98bcc to bb827f2 Compare August 21, 2023 10:59

haakonvt force-pushed the add-pydoclint branch from 5323343 to 296e291 Compare August 21, 2023 17:17

doctrino reviewed Aug 22, 2023

View reviewed changes

erlendvollset approved these changes Aug 22, 2023

View reviewed changes

haakonvt force-pushed the add-pydoclint branch from 49f6ae1 to 8bd3e82 Compare August 22, 2023 21:30

haakonvt changed the title ~~Add pydoclint~~ Overhaul docs, add pydoclint and custom formatter Aug 22, 2023

haakonvt force-pushed the add-pydoclint branch from a537899 to 6ee53fc Compare August 23, 2023 14:41

erlendvollset reviewed Aug 24, 2023

View reviewed changes

haakonvt added 3 commits August 24, 2023 19:24

add pydoclint, run autoupdate, cleanup yaml fmt

2e660cc

run updated ruff and black

8d81c0b

add configuration for pydoclint

938db8d

haakonvt and others added 24 commits August 24, 2023 19:25

Also parse static/cls methods in DocFmt. Update pydoclint with bugfix

9e5069c

Fix static/cls methods, manual fixes of raise stmts

78dec90

Skip checking 'raise' in docstrings, hold off for sep PR

ae278a9

Revert "__call__ doesnt use yield"

3d14546

This reverts commit 600466e.

Fix final errors manually....

1c89e6a

various fixes, reverts, tests, pydoclint config

9d71ef2

fix mypy issue in RawTablesAPI.__call__

ca5d6bc

update pydoclint (two issues raised was fixed)

fe93db4

Apply suggestions from code review

19a9e34

Co-authored-by: Anders Albert <[email protected]>

PR feedback, tweaks

3ce99f3

move pydoclint config to pre-commit config

6f98574

upgrade type hints

8a5dc2d

fix failing test, revert keep-runtime-typing

c0b824a

mypy doesnt understand list, wtf

bb98ca2

lets not start annot with None...

1d492cb

fix the one test with get_type_hints (broken)

db56666

Revert ec38b45, move list method to end of cls def

5c89c3b

Change yields-annotation behaviour

171ac1d

Manually fix leftovers after a messy rebase

7963ced

extend allowed sort types in _list* methods

48b128f

Update pydoclint after it was bugfixed for DOC403

1cdeaef

Bump pydoclint after yield-bugfix

104f63f

bump pydoclint again 0.2.4

b4fa9d0

pr feedback, remove cast in RawTablesAPI

6e07684

haakonvt force-pushed the add-pydoclint branch from 13eb797 to 6e07684 Compare August 24, 2023 20:10

haakonvt added 2 commits August 24, 2023 23:04

speed up custom-checks with pass_filenames=true

2fa4e77

move frozenset change to sep pr

0beb8fa

haakonvt merged commit 8a9beed into master Aug 25, 2023
7 checks passed

haakonvt deleted the add-pydoclint branch August 25, 2023 10:38

haakonvt mentioned this pull request Mar 4, 2024

Basic methods for multipart file upload #1657

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul docs, add `pydoclint` and custom formatter #1314

Overhaul docs, add `pydoclint` and custom formatter #1314

haakonvt commented Aug 12, 2023 •

edited

Loading

haakonvt commented Aug 17, 2023 •

edited

Loading

codecov bot commented Aug 21, 2023 •

edited

Loading

haakonvt commented Aug 21, 2023

doctrino left a comment

doctrino Aug 22, 2023

haakonvt Aug 22, 2023

erlendvollset left a comment

erlendvollset Aug 22, 2023

haakonvt Aug 22, 2023

erlendvollset left a comment

erlendvollset Aug 24, 2023

haakonvt Aug 24, 2023

haakonvt Aug 25, 2023

erlendvollset Aug 24, 2023

haakonvt Aug 24, 2023

Overhaul docs, add pydoclint and custom formatter #1314

Overhaul docs, add pydoclint and custom formatter #1314

Conversation

haakonvt commented Aug 12, 2023 • edited Loading

Description

Note

Checklist:

haakonvt commented Aug 17, 2023 • edited Loading

codecov bot commented Aug 21, 2023 • edited Loading

Codecov Report

haakonvt commented Aug 21, 2023

doctrino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erlendvollset left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erlendvollset left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Overhaul docs, add `pydoclint` and custom formatter #1314

Overhaul docs, add `pydoclint` and custom formatter #1314

haakonvt commented Aug 12, 2023 •

edited

Loading

haakonvt commented Aug 17, 2023 •

edited

Loading

codecov bot commented Aug 21, 2023 •

edited

Loading