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

Add Ruff D rule #982

Open
wants to merge 2 commits into
base: enhancement/rx-docs
Choose a base branch
from

Conversation

MarcSkovMadsen
Copy link
Collaborator

@MarcSkovMadsen MarcSkovMadsen commented Nov 23, 2024

Builds on #977. So review that one first.

The only thing I've touched is in pyproject.toml. The rest is auto fixed by Ruff when I run ruff check.

"D105", # Missing docstring in magic method"
"D107", # Missing docstring in `__init__`"
"D200", # One-line docstring should fit on one line"
"D203", # one-blank-line-before-class and `no-blank-line-before-class` (D211) are incompatible.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm proposing D211 over D203. What I care most about is that we have a rule enforcing consistency.

Copy link
Member

Choose a reason for hiding this comment

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

Do you even need to pick one of them? D203 is disabled by default and D211 enabled by default for the numpy convention. At least that's what the docs say https://docs.astral.sh/ruff/rules/blank-line-before-class/.

"D200", # One-line docstring should fit on one line"
"D203", # one-blank-line-before-class and `no-blank-line-before-class` (D211) are incompatible.
"D205", # 1 blank line required between summary line and description
"D212", # multi-line-summary-first-line. Alternative is to ignore D213 `multi-line-summary-second-line`.
Copy link
Collaborator Author

@MarcSkovMadsen MarcSkovMadsen Nov 23, 2024

Choose a reason for hiding this comment

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

I'm proposing D213 over D212. Again - I don't care so much about the specific rule as I care about having a rule and consistency.

Copy link
Member

Choose a reason for hiding this comment

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

No opinion on this one.

Copy link

codecov bot commented Nov 23, 2024

Codecov Report

Attention: Patch coverage is 54.54545% with 5 lines in your changes missing coverage. Please review.

Project coverage is 87.36%. Comparing base (1868240) to head (7af55fb).

Files with missing lines Patch % Lines
param/serializer.py 0.00% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #982   +/-   ##
=======================================
  Coverage   87.35%   87.36%           
=======================================
  Files           9        9           
  Lines        4935     4938    +3     
=======================================
+ Hits         4311     4314    +3     
  Misses        624      624           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@hoxbro hoxbro changed the base branch from main to enhancement/rx-docs November 23, 2024 08:14
@hoxbro
Copy link
Member

hoxbro commented Nov 23, 2024

Just changing base branch so it easier to review

@@ -25,37 +25,37 @@ class Serialization:

@classmethod
def schema(cls, pobj, subset=None):
raise NotImplementedError # noqa: unimplemented method
raise NotImplementedError
Copy link
Member

Choose a reason for hiding this comment

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

I'm unsure why this has changed; maybe it's because of --unsafe-fixes. I don't think they should be added back

Copy link
Collaborator Author

@MarcSkovMadsen MarcSkovMadsen Nov 23, 2024

Choose a reason for hiding this comment

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

Ruff complained that specific rules like D123 should be used with noqa. So I removed them. Forgot to explain that.

@@ -138,6 +139,7 @@ async def async_wait_until(fn, timeout=5000, interval=100):
Waiting interval, by default 100

Adapted from pytest-qt.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this empty line, as other docstrings do not necessarily have it. But if this is what ruff say, so be it.

"D105", # Missing docstring in magic method"
"D107", # Missing docstring in `__init__`"
"D200", # One-line docstring should fit on one line"
"D203", # one-blank-line-before-class and `no-blank-line-before-class` (D211) are incompatible.
Copy link
Member

Choose a reason for hiding this comment

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

Do you even need to pick one of them? D203 is disabled by default and D211 enabled by default for the numpy convention. At least that's what the docs say https://docs.astral.sh/ruff/rules/blank-line-before-class/.

"D200", # One-line docstring should fit on one line"
"D203", # one-blank-line-before-class and `no-blank-line-before-class` (D211) are incompatible.
"D205", # 1 blank line required between summary line and description
"D212", # multi-line-summary-first-line. Alternative is to ignore D213 `multi-line-summary-second-line`.
Copy link
Member

Choose a reason for hiding this comment

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

No opinion on this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants