nix: setup linter to lint python files (pt II)

[jenstroeger]

@taimoorzaeem the PR we discussed here.

A few additions:

• I installed and ran isort to put the imports into a consistent order:

  isort nix/ test/
  

  I used isort’s defaults, without a configuration which mildly conflicts with the next step (see here).
• I also ran

  ruff format --config ruff.toml
  

  to format the code consistently using the default Black settings.
• This is the configuration I used for ruff:

  # Assume Python 3.13.
  target-version = "py313"
  
  # Code formatting.
  line-length = 120
  indent-width = 4
  
  # Ignore the following lint.
  lint.ignore = [
      "E741",  # https://docs.astral.sh/ruff/rules/ambiguous-variable-name/ (it's ok to use `l` for a variable name)
      "F811",  # https://docs.astral.sh/ruff/rules/redefined-while-unused/ (conflicts with pytest fixtures)
  ]

Turns out there was much noise for a few fairly straightforward changes, and now the code is cleaner:

(venv) postgrest > ruff check --config ruff.toml 
All checks passed!

Out of curiosity I also ran perflint:

(venv) postgrest > perflint nix/ test/

which came back with a few suggestions. Happy to add some of those (perhaps not all) if an iota of performance increase matters 🤓

Also, I did not actually run these changes… 😇

[taimoorzaeem]

I also ran
ruff format --config ruff.toml

No need for the format, we already have black styling setup.

  # Ignore the following lint.
  lint.ignore = [
      "E741",  # https://docs.astral.sh/ruff/rules/ambiguous-variable-name/ (it's ok to use `l` for a variable name)
      "F811",  # https://docs.astral.sh/ruff/rules/redefined-while-unused/ (conflicts with pytest fixtures)
  ]
  ```

Why ignore E741? I think l is ambiguous and should be renamed to line instead.

Overall, this looks nice, but this looks like a lot of change and it is a bit harder to judge and review. @jenstroeger How about only clearing one warning E405 (star import) for now. This way it would be easy and quick to review and merge.

[jenstroeger]

No need for the format, we already have black styling setup.

Ruff claims to replace Black as well, if you want to risk that…

Why ignore E741? I think l is ambiguous and should be renamed to line instead.

Fairnuff.

@jenstroeger How about only clearing one warning E405 (star import) for now. This way it would be easy and quick to review and merge.

You should be able to cherry-pick commit 7ceb260 for that, no? Or should I open a separate PR for that you can merge?

[taimoorzaeem]

[...] Or should I open a separate PR for that you can merge?

Yes please do with just one commit/change, so we can review easily.

Also, we follow commit message conventions, fix: ... is for bug fixes, so here the commit message could be something like test(pytest): remove star imports to please ruff linter. This would be more descriptive. 😇

[jenstroeger]

Yes please do with just one commit/change, so we can review easily.

Draft PR #4382

Also, we follow commit message conventions, fix: ... is for bug fixes, so here the commit message could be something like test(pytest): remove star imports to please ruff linter. This would be more descriptive. 😇

Agreed, I’m a fan of conventional commits myself 👍🏼