Skip to content

Conversation

@Dvdandrades
Copy link

@Dvdandrades Dvdandrades commented Dec 24, 2025

Summary of the changes / Why this is an improvement

Add Ruff to dev dependencies
Remove black from dev dependencies
Update GitHub Actions CI lint job to run Ruff instead of black
Update DEVELOPMENT.md

Run Ruff and fix imports declarations and f string

Add Ruff config in pyproject.toml with just the line-lenght. It can be improved with more rules

Checklist

Copy link
Member

@surister surister left a comment

Choose a reason for hiding this comment

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

Hey @Dvdandrades, Thank you for the PR, I approved the CI run and as you can see the command uv run ruff check trips the CI because of the new lint errors.

Now that ruff is up and running, you can run uv run format to fix most 'fixable' errors, then manually fix anything if it cannot do it automatically, if you need any further advice let me know 👌

pyproject.toml Outdated
"django-stubs>=5.1.3",
"pytest-cratedb>=0.4.0",
"requests>=2.32.3",
"ruff>=0.14.10",
Copy link
Member

@amotl amotl Dec 25, 2025

Choose a reason for hiding this comment

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

Thanks for this patch. Writing down the dependency for ruff is better like this, to not cause any behaviour drift, due to new formatting rules, and deprecations of older ones, coming from future releases.

Suggested change
"ruff>=0.14.10",
"ruff<0.15",

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the clarification. I was checking other repositories and didn't understand this at first when I saw it. Now I understand why it's done this way.

Copy link
Member

@amotl amotl Dec 26, 2025

Choose a reason for hiding this comment

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

Dependabot also understands this paradigm, and will submit corresponding patches, so maintainers can either merge without ado (if nothing changes), or accompany the PR with required adjustments about code formatting rules coming from a newer minor version of ruff.

uv.lock Outdated
Copy link
Member

@amotl amotl Dec 25, 2025

Choose a reason for hiding this comment

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

The Django adapter should also be treated as a library, because it is usually not installed standalone, but pulled in as a dependency. In this spirit, the lock file can be removed. Package consumers can't use it anyway.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the information. So, from what I understand, in cases where the project is a library, it's good practice to include the lock file in the .gitignore file or simply remove it, right?

@Dvdandrades
Copy link
Author

I believe all the changes have been made correctly. I've updated the branch so that ruff could fix the missing error. I've updated the ruff dependency and added the .lock file to the .gitignore to ensure consistency with other repositories.

Copy link
Member

@amotl amotl left a comment

Choose a reason for hiding this comment

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

Excellent, thanks. Please squash 1 your commits before merging.

Footnotes

  1. Nit: Doesn't need to be a single commit, do it at your disposal, but thinning out would be good (e.g. one for adding ruff + formatting, and another one for removing the lock file). Thanks!

opts["OPTIONS"] = {"verify_ssl_cert": False}
c = DatabaseWrapper(opts).get_connection_params()
assert c["verify_ssl_cert"] == False
assert not c["verify_ssl_cert"]
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Just in case you want to be explicitly checking for False. In this case, an identity match is applicable, contrary to the typical equality match.

Suggested change
assert not c["verify_ssl_cert"]
assert c["verify_ssl_cert"] is False

Copy link
Author

Choose a reason for hiding this comment

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

Suggested change done. Thank you so much for the explanations. I've learned a lot.

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.

Change linter, from black to ruff

3 participants