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

Remove Precommits #391

Closed
wants to merge 3 commits into from
Closed

Remove Precommits #391

wants to merge 3 commits into from

Conversation

TreyWW
Copy link
Owner

@TreyWW TreyWW commented May 28, 2024

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.

Copy link
Contributor

github-actions bot commented May 28, 2024

Dependency Review

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

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Manifest Files

@TreyWW TreyWW force-pushed the enhance/remove_precommits branch from 08e12d6 to 889fcba Compare May 28, 2024 22:02
@TreyWW
Copy link
Owner Author

TreyWW commented May 28, 2024

  • needs to remove poetry deps

@Domejko
Copy link
Contributor

Domejko commented May 29, 2024

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
Copy link
Owner Author

TreyWW commented May 29, 2024

Does prettier support Django templates though @Domejko?

@Domejko
Copy link
Contributor

Domejko commented May 29, 2024

There is jinja2 plugin.

@TreyWW
Copy link
Owner Author

TreyWW commented May 29, 2024

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
Copy link
Contributor

Domejko commented May 29, 2024

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
Copy link
Owner Author

TreyWW commented May 29, 2024

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
Copy link
Contributor

Domejko commented May 29, 2024

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
Copy link
Owner Author

TreyWW commented May 30, 2024

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 TreyWW closed this May 30, 2024
@TreyWW
Copy link
Owner Author

TreyWW commented May 30, 2024

(moved to #395)

@TreyWW TreyWW reopened this May 30, 2024
@TreyWW TreyWW closed this May 30, 2024
@TreyWW TreyWW deleted the enhance/remove_precommits branch June 19, 2024 18:18
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