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

Enable linting with ruff #8

Merged
merged 9 commits into from
Jan 7, 2025
Merged

Enable linting with ruff #8

merged 9 commits into from
Jan 7, 2025

Conversation

alexfikl
Copy link
Contributor

@alexfikl alexfikl commented Jan 6, 2025

This adds linting with ruff and fixes all the errors that showed up.

The plan is mostly to figure out a configuration that works here and then I'll slowly update the other projects as well.

@Wrzlprmft
Copy link
Collaborator

Thank you. Overall, this looks very good.

A few requests:

  • Do not de-indent empty lines within an indented block. The point of this is that visible indentation marks are consistent.
  • In docstrings, shouldn’t be replaced with '. (What’s the point of having proper Unicode support when you don’t use it?)
  • I know what __all__ does, but I don’t see a particular need for it here. If there isn’t any, I would drop it as it just complicates things.

The isort changes in 62fedc3 probably need a closer look since they reformat the imports quite a bit. ruff has a bunch of settings for isort, so we can probably find a style that fits.

I am not worried about the order of imports as I don’t see a way to botch something here that wouldn’t immediately fail a test.

@alexfikl
Copy link
Contributor Author

alexfikl commented Jan 7, 2025

Thanks for taking a look!

  • Do not de-indent empty lines within an indented block. The point of this is that visible indentation marks are consistent.

I'm not sure I follow your point. If you have some code like

something = 0
if condition:
	something = 3
>	>
something += 5

On the empty line between the statements, is the whitespace tab supposed to be at the same indentation as the if block contents or at the same indentation as the outer block?

I'll remove the commit that fixes the blank-line-with-whitespace (W293) rule in ruff, which seems to be what this is about.

  • In docstrings, shouldn’t be replaced with '. (What’s the point of having proper Unicode support when you don’t use it?)

The issue that ruff is complaining about here is ambiguous-unicode-character-string (RUF001), not general unicode usage. I mostly agree with that, i.e. if there is an ASCII character that's more commonly used, it's mostly just confusing to use other unicode characters (e.g. I'm not even sure how to write the apostrophe on my keyboard).

  • I know what __all__ does, but I don’t see a particular need for it here. If there isn’t any, I would drop it as it just complicates things.

It was mostly meant to avoid some unused-import (F401) warnings. I'll remove them altogether instead.

I am not worried about the order of imports as I don’t see a way to botch something here that wouldn’t immediately fail a test.

I mostly meant the style that that's imposed by ruff here. Going forward it will complain if the imports are not formatted this way (e.g. sorted alphabetically). It's not set in stone though, obviously, so you can tweak it later 😁

@Wrzlprmft
Copy link
Collaborator

  • Do not de-indent empty lines within an indented block. The point of this is that visible indentation marks are consistent.

I'm not sure I follow your point. If you have some code like

something = 0
if condition:
	something = 3
>	>
something += 5

On the empty line between the statements, is the whitespace tab supposed to be at the same indentation as the if block contents or at the same indentation as the outer block?

In your example, you have an extra indentation, which should not happen at all. Also, it’s not within an indented block, but after one. Such indentations can be completely removed. What I was talking about is something like this:

something = 0
if condition:
>	something = 3
>	
>	foo(something)

something += 5

Here, Ruff has removed the indent in the line preceding foo(something), which I consider bad.

I'll remove the commit that fixes the blank-line-with-whitespace (W293) rule in ruff, which seems to be what this is about.

That fixed the issue.

  • In docstrings, shouldn’t be replaced with '. (What’s the point of having proper Unicode support when you don’t use it?)

The issue that ruff is complaining about here is ambiguous-unicode-character-string (RUF001), not general unicode usage. I mostly agree with that, i.e. if there is an ASCII character that's more commonly used, it's mostly just confusing to use other unicode characters (e.g. I'm not even sure how to write the apostrophe on my keyboard).

Well, I agree for code, where such ambiguities can cause problems. However, we are talking about a docstring here, which is not intended to be parsed as code, so I don’t find the argument convincing.  produces the correct apostrophe (and closing single quotation mark in English) in most fonts; ' is a historic compromise to capture several characters in one.

  • I know what __all__ does, but I don’t see a particular need for it here. If there isn’t any, I would drop it as it just complicates things.

It was mostly meant to avoid some unused-import (F401) warnings. I'll remove them altogether instead.

The last sentence refers to the imports right? In that case, I agree.

I am not worried about the order of imports as I don’t see a way to botch something here that wouldn’t immediately fail a test.

I mostly meant the style that that's imposed by ruff here. Going forward it will complain if the imports are not formatted this way (e.g. sorted alphabetically). It's not set in stone though, obviously, so you can tweak it later 😁

Okay.

@alexfikl alexfikl marked this pull request as draft January 7, 2025 17:54
@alexfikl
Copy link
Contributor Author

alexfikl commented Jan 7, 2025

I'll remove the commit that fixes the blank-line-with-whitespace (W293) rule in ruff, which seems to be what this is about.

That fixed the issue.

This should be fixed in the latest version.

Well, I agree for code, where such ambiguities can cause problems. However, we are talking about a docstring here, which is not intended to be parsed as code, so I don’t find the argument convincing. produces the correct apostrophe (and closing single quotation mark in English) in most fonts; ' is a historic compromise to capture several characters in one.

Fair enough, I've removed all those changes. I've also found how to type that typographic apostrophe on my keyboard: on Linux, it works with the Compose key like Compose + ' + > 😁

@alexfikl alexfikl marked this pull request as ready for review January 7, 2025 18:21
@Wrzlprmft Wrzlprmft merged commit 8ea6c86 into neurophysik:master Jan 7, 2025
5 checks passed
@alexfikl alexfikl deleted the ruff branch January 7, 2025 20:12
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.

2 participants