Skip to content

Small things #260

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

Closed
wants to merge 69 commits into from
Closed

Conversation

petsagouris
Copy link

Basically setting up ruff for linting on Django Ledger and doing initial passes for around 20 or so rules.

At some point that the project will require it can be very easily formatted in a unformed and automatic way.

I fought my way through multiple exceptions to get this to work. I don't really know why. Now it works just fine, all the backend functions work too.
Using an in-memory database and disabling DEBUG and TEMPLATE_DEBUG.
Now there is no exceptions.
- unnecessary-double-cast-or-process (C414)
- unnecessary-list-comprehension-dict (C404)
- unnecessary-list-comprehension-set (C403)
- blank-line-with-whitespace (W293)
- implicit-return (RET503)
@elarroba elarroba self-assigned this Apr 4, 2025
@elarroba
Copy link
Member

elarroba commented Apr 4, 2025

Hello @petsagouris. Thank you for your PR.
Other than formatting is there a particular bug-fix or feature implementation you are contributing to?

@petsagouris
Copy link
Author

I've tried to get no changes to the formatting of the code.
I'm aware that changing formatting can be a real mess up without prior discussion.

a7f8c09 introduces a ruff configuration for the linter and this PR is mostly abou fixing up linter issues.
Some were important because they contribute to the uniformity of the code and apply some best practices.
You can search the code in parenthesis at the end of each commit in the ruff website to check why the linter has the rule.
There is more rule violations but I'm putting this PR to see how receptive you, the maintainer, would be to this type of contributions.

9abb46d gets vs code to discover the tests and run then easier.

There is more things I wanna do PRs for in django-ledger but cleaning up linter errors gives an excellent chance to familiarize with the code base. I was thinking, fix up the failing tests, take care of internationalization and then get to deal with some of those TODOs. :)

@elarroba
Copy link
Member

@petsagouris, thank you for your contribution, and I appreciate the spirit behind it.

However, I’m concerned about the idea of changing numerous lines and files simultaneously. Especially if these changes aren’t directly related to a new feature, enhancement, or bug fix.

While I support code refactoring and linting, I suggest keeping your refactored branch separate. If that helps, you can create a separate branch for your refactoring. However, please ensure that your pull request is limited to the files that directly relate to the main purpose of your contribution.

I look forward to seeing your contributions.

Miguel

@petsagouris
Copy link
Author

I understand completely the sentiment, it is quite a lot to oversee. I'll keep the branch for now but I'm closing this PR since it is not really helping you out.

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