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

Lint templates #597

Merged
merged 6 commits into from
Aug 10, 2024
Merged

Lint templates #597

merged 6 commits into from
Aug 10, 2024

Conversation

jeriox
Copy link
Contributor

@jeriox jeriox commented Jul 30, 2024

closes #586 closes #580

@jeriox jeriox requested a review from frcroth July 30, 2024 13:09
@jeriox
Copy link
Contributor Author

jeriox commented Jul 30, 2024

@SilvanVerhoeven I removed prettier, as it kept messing with the indention that djhtml did and created an endless loop of reformatting. Could you please have a look if you can restrict prettier to only handle pure js files?

.pre-commit-config.yaml Outdated Show resolved Hide resolved
myhpi/core/templates/core/minutes_list.html Outdated Show resolved Hide resolved
myhpi/static/js/print_processor.js Show resolved Hide resolved
@jeriox jeriox force-pushed the lint-templates branch 2 times, most recently from f68309c to 6fc0eda Compare July 30, 2024 13:22
@SilvanVerhoeven
Copy link
Collaborator

SilvanVerhoeven commented Jul 30, 2024

I removed prettier, as it kept messing with the indention that djhtml did and created an endless loop of reformatting. Could you please have a look if you can restrict prettier to only handle pure js files?

Seems like we try to lint the same file types here. I think we need to make a decision which formatter should do what. At least for JS I have a slight preference for prettier, as that has proper IDE support, thus allows format on save.

grafik

Maybe DjLint could be the perfect fit? It seems to have support for django plus IDE extensions

@jeriox

@SilvanVerhoeven
Copy link
Collaborator

@jeriox This also closes #580, correct?

@dropforge
Copy link
Collaborator

Just a pointer that pre-commit's types_or includes multiple extensions for each type, see their identify library.

@jeriox
Copy link
Contributor Author

jeriox commented Jul 31, 2024

@jeriox This also closes #580, correct?

oh yeah, those are duplicates. I thought we didn't have an issue for it yet and the search also came up empty when I searched for "template" and "linter" ^^

@jeriox
Copy link
Contributor Author

jeriox commented Jul 31, 2024

Seems like we try to lint the same file types here. I think we need to make a decision which formatter should do what. At least for JS I have a slight preference for prettier, as that has proper IDE support, thus allows format on save.

In my understanding from monday we agreed that prettier should only lint pure .js files and djhtml should lint the templates. I strongly favor djhtml for linting the templates as it properly understands django syntax and lints inline js and css. Djhtml does not lint .js files.

Maybe DjLint could be the perfect fit? It seems to have support for django plus IDE extensions

I have no experience with DjLint and am used to djhtml, but that shouldn't be the problem here. But I'm not sure whether this solves the problem at hand, which is prettier messing with the templates, as DjLint also only lints templates according to their docs.
On the IDE integration part: I personally hate such "format on save" tools as I don't want them to mess with my code all the time while developing. Formatting before commiting is the better approach IMO, but that is probably just my personal opinion. (and a also don't want to install dozens of extensions in my IDE if they are available in the first place)
Running a specified command (such as djhtml) on save is not possible with your IDE, just extensions?
just to be clear, I don't want to oppose a tool that supports it, it just doesn't convince me to switch to another tool just for that reason

@SilvanVerhoeven
Copy link
Collaborator

SilvanVerhoeven commented Aug 4, 2024

In my understanding from monday we agreed that prettier should only lint pure .js files and djhtml should lint the templates.

I understood the same :) We never talked about .css/.scss files, tho. I feel like Prettier is the more common choice here, do you agree?

Djhtml does not lint .js files.

I just tested it – due to id: djjs, it does. Same for id: djcss. Removing both does not affect formatting of template files.

But I'm not sure whether this solves the problem at hand, which is prettier messing with the templates.

I cannot recreate this – with the following config, .html do not seem to be linted by prettier (as I would expect, since .html files are not listed in tyoes_or).

- repo: https://github.com/rtts/djhtml
    rev: "3.0.6"
    hooks:
      - id: djhtml
  - repo: local
    hooks:
      - id: prettier-eslint
        name: Prettier and ESLint
        entry: prettier-eslint --write --list-different
        language: node
        types_or: [javascript, css, markdown, yaml]
        additional_dependencies: ["[email protected]"]

I personally hate such "format on save" tools as I don't want them to mess with my code all the time while developing.

It makes writing JS/CSS (and even templates) much easier for me, as I don't need to care about indentation/can let the IDE clean up everything after I wrote a bit – and then directly proceed coding from a "clean" state.

Don't want to install dozens of extensions in my IDE if they are available in the first place.

I would never argue against the pre-commit hook, it is very good :) But I'd very much appreciate the option to have "format on save".

Running a specified command (such as djhtml) on save is not possible with your IDE, just extensions?

Only after installing an extension for that and configuring it to run a command – and it seems running that command in a venv is a further headache.

DjLint only lints templates according to their docs.

Mhh, to me it seems like they also have a formatter?
I think I haven't entirely understood the disadvantages of DjLint, except for missing experience. Could you reiterate this again, please? :)

@SilvanVerhoeven SilvanVerhoeven force-pushed the lint-templates branch 3 times, most recently from 20a2117 to b9b5abf Compare August 4, 2024 20:34
@SilvanVerhoeven
Copy link
Collaborator

@jeriox

So my proposal is this:

Advantages:

  • djLint formats code, not only indents (djHTML only indents) → ensures more consistent code style; especially helpful in this project with so many contributors with different backgrounds
  • djLint seems to detect more errors – I found several issues in out code base like missing closing tags after using it
  • djLint has VSCode plugin support – can be easily used on save and shows warnings in the IDE, with the same configuration as used in the pre-commit hook

What do you think of this setup?

Copy link
Contributor Author

@jeriox jeriox left a comment

Choose a reason for hiding this comment

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

apart from the loops I'm fine with this setup (can't approve as this is my PR)

myhpi/core/templates/core/minutes.html Outdated Show resolved Hide resolved
@SilvanVerhoeven SilvanVerhoeven enabled auto-merge (rebase) August 10, 2024 08:36
@SilvanVerhoeven SilvanVerhoeven merged commit d13eb92 into main Aug 10, 2024
8 checks passed
@SilvanVerhoeven SilvanVerhoeven deleted the lint-templates branch August 10, 2024 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add template linter Integrate HTML formatter
4 participants