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

gh-248: a few fixes for ignored and recommended ruff rules #360

Merged
merged 3 commits into from
Oct 15, 2024

Conversation

Saransh-cpp
Copy link
Member

Fixed:

  • missing-trailing-comma
  • commented-out-code
  • single-line-implicit-string-concatenation
  • numpy-legacy-random
  • pytest-raises-too-broad

Refs: #248

@Saransh-cpp Saransh-cpp added the maintenance Maintenance: refactoring, typos, etc. label Oct 14, 2024
@Saransh-cpp Saransh-cpp self-assigned this Oct 14, 2024
pyproject.toml Outdated
@@ -160,11 +156,10 @@ lint.per-file-ignores = {"__init__.py" = [
"ANN201", # TODO: issing-return-type-undocumented-public-function
"ANN202", # TODO: missing-return-type-private-function
"D100", # undocumented-public-module
"D103", # TODO: undocumented-public-function
Copy link
Member Author

Choose a reason for hiding this comment

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

I think it should be okay to not have docstrings in the test functions

Copy link
Member

Choose a reason for hiding this comment

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

I do think it's something we should work towards

pyproject.toml Outdated Show resolved Hide resolved
Copy link
Member

@paddyroddy paddyroddy left a comment

Choose a reason for hiding this comment

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

Thanks for these, and I prefer the look of the comma options. However, see my concerns about ruff-format and conflicting rules

@@ -121,12 +121,8 @@ lint.ignore = [
"ANN002", # TODO: missing-type-args
"ANN003", # TODO: missing-type-kwargs
"ANN201", # TODO: missing-return-type-undocumented-public-function
"COM812", # missing-trailing-comma (ruff-format recommended)
Copy link
Member

Choose a reason for hiding this comment

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

You have to leave this one in, that is why I didn't write TODO... In fact, there are more than I thought - just found the docs https://docs.astral.sh/ruff/formatter/#conflicting-lint-rules

"D203", # one-blank-line-before-class
"D212", # blank-line-before-class
"ERA001", # TODO: commented-out-code
"ISC001", # single-line-implicit-string-concatenation (ruff-format recommended)
Copy link
Member

Choose a reason for hiding this comment

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

You have to leave this one in, that is why I didn't write TODO... In fact, there are more than I thought - just found the docs https://docs.astral.sh/ruff/formatter/#conflicting-lint-rules

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I thought the recommendation meant adding it at some point. My bad!

Copy link
Member

Choose a reason for hiding this comment

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

Could you make an issue to look into these conflicting rules?

pyproject.toml Outdated
@@ -160,11 +156,10 @@ lint.per-file-ignores = {"__init__.py" = [
"ANN201", # TODO: issing-return-type-undocumented-public-function
"ANN202", # TODO: missing-return-type-private-function
"D100", # undocumented-public-module
"D103", # TODO: undocumented-public-function
Copy link
Member

Choose a reason for hiding this comment

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

I do think it's something we should work towards

Copy link
Member

Choose a reason for hiding this comment

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

Can we add a random seed to default_rng?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are introducing a new concept into the example (creating a RNG), then it should ideally happen explicitly and somewhere near the top.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are comments like this one in all of the examples, and they no longer make sense because now the user does not have to import N different modules.

Maybe we could comment something like

# almost all GLASS functionality is available from the `glass` namespace

@Saransh-cpp Saransh-cpp changed the title gh-248: a few fixes for ignored ruff rules gh-248: a few fixes for ignored and recommended ruff rules Oct 15, 2024
@Saransh-cpp
Copy link
Member Author

Thanks for the reviews! This should be better now.

Copy link
Member

@paddyroddy paddyroddy left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for sorting

@Saransh-cpp Saransh-cpp merged commit cfe3245 into main Oct 15, 2024
14 checks passed
@Saransh-cpp Saransh-cpp deleted the saransh/ruff-fixes branch October 15, 2024 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Maintenance: refactoring, typos, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants