adamchainz / djade

A Django template formatter.
MIT License
256 stars 4 forks source link

Split `{% load %}` tags one per line #33

Open ghickman opened 1 month ago

ghickman commented 1 month ago

Description

First of all, thank you for building this!

Are you interested in adding any configuration options?

I'm currently working on a project where most templates pull in a lot of tags. Many of those templates use a single load line, and the list is easily off the side of your screen. I'm moving the project to one tag per line to try and get a handle on the situation, but it's also my preference for ease of review.

Would you consider adding a config option to split or combine load lines?

adamchainz commented 1 month ago

Are you interested in adding any configuration options?

Generally, no, the idea is to make this like Black.

I advocated for one-per-line in the style guide discussion but swung around to merging them because templates don't typically load that many libraries.

Can you alleviate your problem by moving some libraries to builtins?

ghickman commented 1 month ago

templates don't typically load that many libraries.

I wish that were the case for us! We're in a tricky position where devs have been adding libraries to the templates but not removing them for so long that now folks see it as an impossible mountain to climb. We'll get there, but this would make life easier 😁 As you say in the discussion, it helps with diffs, which might make it worth an option?

Can you alleviate your problem by moving some libraries to builtins?

This is also on my list! (Yet again, thank you for the great articles you write :))

gav-fyi commented 1 month ago

...adding libraries to the templates but not removing them for so long...

I wonder how difficult it would be to find unused load tags? Like we have for unused imports. 🤔 It feels like it could be a new fixer.

adamchainz commented 1 month ago

It's not going to be possible to detect unused load tags within Djade, since it operates on text only. Determining which tags and filters come from a load tag requires execution of the library. It should be feasible as a management command.

adamchainz commented 1 month ago

As you say in the discussion, it helps with diffs, which might make it worth an option?

If a bunch of people vote in favour, sure.

Another thing that could work is if multiline tag support is ever added to Django, then we could do:

{% load
  albumen
  shell
  yolk
%}

There have been several starts on this. I could only find an older one after a quick search: https://github.com/django/django/pull/2556 . I think @tim-schilling may have been the last to work on this??

tim-schilling commented 4 weeks ago

:wave: Hi! I'm not sure I worked on that though. It's not ringing any bells for me anywhere and I've looked around for a bit.

adamchainz commented 4 weeks ago

Okay, my memory failed me, sorry for the tag then.

adamchainz commented 4 weeks ago

I only found these old wontfix tickets: https://code.djangoproject.com/ticket/3888 , https://code.djangoproject.com/ticket/8652 . I think there was a forum discussion or PR in the last few years but can't find it.

UnknownPlatypus commented 4 weeks ago

This is just my two cents, but I would also prefer one load tag per line, ordered alphabetically to help with diffs and merge conflicts (same reasoning as reorder-python-imports)

knyghty commented 3 weeks ago

I would also prefer this but I wouldn't say it's super important to me.

meshy commented 3 weeks ago

It's not a deal-breaker for me, but I would also vote for one-load-per-line. It makes it easier to search for where loads are used.

I also encounter templates with a lot of loads in them, so I sympathise with the "too long line" argument.

I agree with the decision to not allow configuration.

I'm not keen on "moving tags to builtins" in most cases. I have worked on codebases that did that (long ago), and it's fair to say we regretted it. It made it exceptionally hard to work out where they were used when it came to removing them.

nessita commented 3 weeks ago

I'm strongly in favor of one load per line, this meaning using the whole {% load name %} construction in each line and not the one proposed in https://github.com/adamchainz/djade/issues/33#issuecomment-2367855533 (since I almost pass out when I saw it :-D).

jefftriplett commented 3 weeks ago

+1 one load-statement per line too. I'm only commenting because Adam asked.

https://forum.djangoproject.com/t/djade-a-template-formatter/35138/3

Edit: The reason I prefer one line per load statement is because it streamlines code reviews and diffs.

joshuadavidthomas commented 3 weeks ago

I'm a big +1 for this style of one load per line.

I'm in agreement about the zero config. I could see one exception made for this rule opening the doors for other exceptions. Even if the answer is always "no", that would still add to the burden of maintaining the tool and I would not want that.

I like the desire to hew as close as possible to Django's house opinions of formatting templates. I just happen to disagree with this one opinion 😄.

However at the end of the day, it's not a huge deal-breaker for me. I'm still planning on adopting djade at work and on my personal projects -- the other Django template formatters I've used are just unbearably slow sometimes, especially on larger templates.

Thanks for your work on this tool Adam!

carltongibson commented 3 weeks ago

I much prefer my load tag as a list on a single line. (I sort those a-z)