Skip to content

Add docstrings to every public mudule, class and function#446

Merged
mathialo merged 11 commits intomasterfrom
doc-strings
Jun 12, 2025
Merged

Add docstrings to every public mudule, class and function#446
mathialo merged 11 commits intomasterfrom
doc-strings

Conversation

@mathialo
Copy link
Copy Markdown
Contributor

This PR enables the D rule for ruff, which requires docstrings on every public module, class and function. It also contains some validation of the docstrings themeselves (such as ensuring that every function parameter is covered, and making sure the docstring adheres to the style guide.

I also intend on adding the DOC rule which adds further validation of the doc strings (such as looking for exceptions being thrown that aren't documented), but that also requires us to run in preview mode which will trigger more linting fixes, so I'll do that in a later PR.

I've also completely turned off the D rule for tests for now. While I think some simple documentation would be good for the tests, I'm trying to keep this PR small (believe it or not...).

This is a massive PR in terms of line changes, but it's only adding or fixing documentation strings, no functional changes are made.

@mathialo mathialo marked this pull request as ready for review June 10, 2025 09:21
@mathialo mathialo requested a review from a team as a code owner June 10, 2025 09:21
@codecov
Copy link
Copy Markdown

codecov bot commented Jun 10, 2025

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 77.46%. Comparing base (ac184d5) to head (0e25205).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
cognite/extractorutils/uploader/time_series.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #446      +/-   ##
==========================================
- Coverage   77.47%   77.46%   -0.01%     
==========================================
  Files          42       42              
  Lines        3511     3510       -1     
==========================================
- Hits         2720     2719       -1     
  Misses        791      791              
Files with missing lines Coverage Δ
cognite/extractorutils/_inner_util.py 94.73% <ø> (ø)
cognite/extractorutils/base.py 51.08% <ø> (ø)
cognite/extractorutils/configtools/__init__.py 100.00% <ø> (ø)
cognite/extractorutils/configtools/_util.py 65.57% <ø> (ø)
cognite/extractorutils/configtools/elements.py 80.09% <ø> (ø)
cognite/extractorutils/configtools/loaders.py 84.09% <ø> (ø)
cognite/extractorutils/configtools/validators.py 72.72% <ø> (ø)
cognite/extractorutils/exceptions.py 77.77% <ø> (ø)
cognite/extractorutils/metrics.py 92.75% <ø> (ø)
cognite/extractorutils/statestore/__init__.py 100.00% <ø> (ø)
... and 27 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@thorkildcognite thorkildcognite added the risk-review-exemption-big-pr Approved exemption for large PR (>500 lines) label Jun 11, 2025
Copy link
Copy Markdown
Contributor

@toondaey toondaey left a comment

Choose a reason for hiding this comment

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

Not a part of this but also saw some errors:

  1. .cognite/extractorutils/exceptions.py - "Unknown" instead of "Unkown".
  2. .cognite/extractorutils/metrics.py - "overriden" instead of "overrided".
  3. ./cognite/extractorutils/unstable/configuration/exceptions.py - "Unknown" instead of "Unkown".

Not sure if we can change this but also a spelling error:
./cognite/extractorutils/uploader/time_series.py - "Sequence" instead of "Sequnce"

mathialo and others added 2 commits June 11, 2025 14:19
Co-authored-by: Babatunde Aromire <babatunde.aromire@cognite.com>
@mathialo mathialo requested a review from toondaey June 11, 2025 12:21
toondaey
toondaey previously approved these changes Jun 11, 2025
Copy link
Copy Markdown
Contributor

@toondaey toondaey left a comment

Choose a reason for hiding this comment

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

👌🏾

@mathialo mathialo added the waiting-for-risk-review Waiting for a member of the risk review team to take an action label Jun 11, 2025
Copy link
Copy Markdown
Contributor

@thorkildcognite thorkildcognite left a comment

Choose a reason for hiding this comment

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

Minor nits to consider

@thorkildcognite
Copy link
Copy Markdown
Contributor

risk review ok -- some small nits pointed out that you might fix, but nothing big.

@thorkildcognite
Copy link
Copy Markdown
Contributor

risk review ok

@mathialo mathialo merged commit 6ec693b into master Jun 12, 2025
6 checks passed
@mathialo mathialo deleted the doc-strings branch June 12, 2025 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk-review-exemption-big-pr Approved exemption for large PR (>500 lines) waiting-for-risk-review Waiting for a member of the risk review team to take an action

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants