TreyWW / MyFinances

MyFinances is a web application that can help you as an individual, or team, manage your finances!
https://docs.myfinances.cloud
GNU Affero General Public License v3.0
100 stars 144 forks source link

Remove Precommits #391

Closed TreyWW closed 4 months ago

TreyWW commented 4 months ago

I believe pre-commits are a nightmare. They slow down development, sometimes cause issues that fully block users from committing (have done this to me many times!) and just don't seem worth it.

We have PR status checks that will block things like linting issues. I'd rather allow users to commit and then guide them to fix issues rather than not letting them commit and be stuck on their own.

github-actions[bot] commented 4 months ago

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Manifest Files

TreyWW commented 4 months ago
Domejko commented 4 months ago

Maybe better solution would be to keep just basic ones like black, djlint/prettier and mypy. I would also consider swapping djLint to Prettier it's much more popular, active and up to date in compare to djLint and covers more formats like css/js/md/yaml. I could implement that but for that to work HTML code need to be fixed because it's a mess, double quotes used everywhere like here

hx-get="{% url "api:base:modal retrieve with context" modal_name="leave_team" context_type="leave_team" context_value=team.id %}"

this cause problems for linting/formatting tools.

TreyWW commented 4 months ago

Does prettier support Django templates though @Domejko?

Domejko commented 4 months ago

There is jinja2 plugin.

TreyWW commented 4 months ago

Ah I see. I still think that precommit are a bad idea though, especially for mypy. At least black and djlint will run in the background without disturbing you. Whereas a mypy test will fully block you. That's the thing I don't want

Domejko commented 4 months ago

Hmm so we could leave it with just Black and for djLint change command to --format instead of --check (to avoid getting blocked by djLint errors) and I could start working on replacing djLint with Prettier but it will take some time since there are dozens of errors in html that have to be fixed before it will be able to run properly.

TreyWW commented 4 months ago

Is there any direct issues with djlint though? I've found it to work wonders. You can also change config to make " turn into single ' but I just had it off cause it's habit to use double quotes lol. We can change some config to make it better, it's a great tool

Domejko commented 4 months ago

Yes but I think it will just replace all " with ' and the issue is that " are used inside " what makes code look like this : Screenshot from 2024-05-29 13-01-05 Closing tag then is seen by IDE as orphan. All that is fixed and handled by browser either way but for the esthetic part and readability it's a stench.

As to djLint vs Prettier I just find Prettier formatting style cleaner plus what I have already mentioned it covers much more formats like js/css/yaml/md what makes just a whole project more standardized/unified. It's also much bigger tool used in almost 8 million repos that is updated much more often and I see it as just having better perspectives for long term use and constant tool improvement.

TreyWW commented 4 months ago

Decided to optimise pre-commits rather then remove them fully. Great ideas @Domejko. As for prettier, I have made a discussion #396 and I'll come back to it at some point - I dont think its too necessary right now.

TreyWW commented 4 months ago

(moved to #395)