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

fmt: Ordered Html attributes #465

Closed
jonerer opened this issue Jan 27, 2024 · 13 comments
Closed

fmt: Ordered Html attributes #465

jonerer opened this issue Jan 27, 2024 · 13 comments
Labels
enhancement New feature or request fmt NeedsDecision Issue needs some more discussion so a decision can be made

Comments

@jonerer
Copy link
Contributor

jonerer commented Jan 27, 2024

I really love that templ helps me structure and format the templ files, like how putting a newline between some Html attributes turns the whole div into a multiline div and lays out the attributes on one line each.

I wish that templ would take this one step further and automatically sort the attributes. Probably by alphabetical order, but maybe there's some other common sorting algorithm. It doesn't matter that much, as long as it's consistent.

The point being that having attributes in a consistent order would really simplify the readability/skimmability of templ files, and that doing this manually is too much work. For instance if you know that an will always be in the same ordering like "class", "name", "id" "type", "value", it would make it much easier to skim the code. For example to find where things might be wrong, what's missing and where to add things. Many times I've accidentally added two different "class" attributes, for instance.

Is there consensus that this would be an improvement? Of course it will have an effect on everybody's code style, which some people might not like. But in generanl I think people are typically in favor of having consistency and automation over having their own specific style.

I think making the change could probably be done around here:

for i := 0; i < len(e.Attributes); i++ {

EDIT:
Desired behaviour:

# cat thing.templ
...
<input type="text" class="mycls" name="weo" value="myvalue"/>
<input name="wuw" value="thing" class="input" type="text"/>
<input name="weow" type="text" class="input input-bordered"/>
...
# templ fmt
# cat thing.templ
...
<input class="mycls" name="weo" type="text"  value="myvalue "/>
<input class="input" name="wuw" type="text" value="thing" />
<input class="input input-bordered" name="weow" type="text" />
....
# Aaah, so neat and tidy
@jonerer
Copy link
Contributor Author

jonerer commented Jan 28, 2024

I made an initial PR. I could update it if you prefer another code style. There are two questions:

  • How do you want the "Name" attribute to be found? Either by casting to known Attribute structs (like ConstantAttribute), by reflection (as done) or by adding a "Name()" method to the Attribute struct
  • Do you have an opinion on where conditional attributes get put? As of the first implementation they're just put last

@joerdav joerdav added enhancement New feature or request cmd fmt and removed cmd labels Jan 29, 2024
@joerdav joerdav changed the title Feature request: Ordered Html attributes fmt: Ordered Html attributes Jan 29, 2024
@joerdav
Copy link
Collaborator

joerdav commented Jan 29, 2024

Thanks for the suggestion. This could definitely be useful, and you've identified where this change should live.

One thing first though is whether this is going to be a benefit or an annoyance for most people. I think I'm on your side of the fence, but I think it could be frustrating if you want your attrs in a specific order.

Could be worth taking notes from other HTML formatters on this. Are there any that do this as a default behavior, is it a big point of contention for them, or does everyone like it?

In terms of approach, I think that reflection should be avoided. All attributes seem to implement fmt.Stringer, so you could order by the result of that as a baseline. And then a special case for conditional attributes and spread attributes, to put them at the end.

@jonerer
Copy link
Contributor Author

jonerer commented Jan 29, 2024

Thanks for the input. I did a little review. There doesn't seem to be a very clear convergence in the ecosystem:

class
id, name
data-*
src, for, type, href, value
title, alt
role, aria-*
tabindex
style

Personally, I can feel that it seems a bit too complicated to do this fine-grained categorization. For me the most important feature is "skimability". And I can't keep that long list in my head. It seems easier to juts think "ok value is usually by the end since v comes late in the alphabet". That's why I suggested just sticking to the alphabet. But I can also understand having the most common attributes like "class", "id", "name" in the front. But perhaps after using it for a while such a deep categorization becomes second nature?

Implementation

Regarding implemetnation, the reason I didn't use "fmt.Stringer" is because of ConditionalAttributes. I might be wrong but I'm quite sure that the Sring() method of ConditionalAttributes will return something like if myCondition {\nclass="input"\n}. So if I would sort on that, the sorting would actually be on the contents of the if statement, not on the attribute name. So therefore I did the reflection to find attribute "Name" where it exists.

I'm also a bit unsure on how to handle the ConditionalAttribute sorting, that's why I just put it last. For instance, how should this be handled?

<div 
   name="myName"
   if cond {
      class="something"
      value="val"
   }
/>

If sorting alphabetically, "class" should end up before "name", but "value" should be after "name". Does that mean we should break up the condition into two sections, and sort them differently? I'd say no. That's too much of an interference. I'd vote to sort the whole attribute block on the first attribute, in this case "class". (and perhaps to do a second sorting, between "class" and "value" as well)

@joerdav
Copy link
Collaborator

joerdav commented Jan 29, 2024

Thanks for the detailed research! Will let it stew for a bit and let others contribute thoughts.

In terms of implementation, I was thinking that Contitionals would be an exception to the Stringer rule, when writing you would check if it's a conditional first, then put it at the end, otherwise use stringer.

I would maybe say to put all ifs at the end, but maybe that's a naive approach.

@JonnyLoughlin
Copy link
Contributor

Would there be any interest in a templ config file? Something if there is no file, defaults are used, otherwise the config file is checked? It may be useful, as templ grows, to allow devs more control over the formatter since opinions around html formatting seem to vary greatly.

Personally I like having id -> class -> [the rest]. If there was a config file, you could specify if orderAttributes is enabled, and also have an optional ordered list where attributes will appear in that order then any not in the list would be alphabetically sorted.

I'm sure the config file comes with trade offs and this solution is more complex, just curious on what others options on this are.

@jonerer
Copy link
Contributor Author

jonerer commented Jan 29, 2024

Would there be any interest in a templ config file? Something if there is no file, defaults are used, otherwise the config file is checked? It may be useful, as templ grows, to allow devs more control over the formatter since opinions around html formatting seem to vary greatly.

AFAIK @a-h and @joerdav have been quite careful to avoid having to have a config file, which has worked great so far. It's nice to have a system that works with default settings and everyone's code comes out looking uniform. Similar to "gofmt"s approach. Most people are happy to have a standard as opposed to having their specific code preferences met.

@joerdav joerdav added the NeedsDecision Issue needs some more discussion so a decision can be made label Jan 30, 2024
@alehechka
Copy link
Contributor

I do see the benefit that can come with having alphabetized attributes on format, however, I think this more of a personal preference feature that I wouldn't agree to be a default behavior. Noting the eslint rules and prettier plugins mentioned in the above review, I think that's more-so evidence that this would be an opt-in feature due to preference than a core of the formatter.

Additionally, there already seem to be mixed opinions on how they should be sorted: fully alphabetical vs. the recommended order in https://codeguide.co/#attribute-order vs. the conditional attributes available in templ.

When using templ myself, I typically have id first, then any htmx/data/aria attributes, then class as the very last attribute since I use tailwind which usually ends up with a large list of classes wrapping to multiple lines.

@joerdav
Copy link
Collaborator

joerdav commented Jan 31, 2024

@jonerer is correct with the config file, it's not something that we'll 100% never have a config file, but I think we're avoiding until we have something very compelling to require one!

I'm also tending to the decision that this should be out of scope for templ, it is a bit "pedantic" for a default behavior.

@a-h Any final thoughts on this, seems like the conversation is converging?

@JonnyLoughlin
Copy link
Contributor

Yeah, I totally agree that introducing a config file for this would be overkill, was interested in the philosophy around one more so (I am a fan of your guys current approach).

I think the best way to go about with would be through editor integrations. I mentioned in the templ slack about using the tailwind sort plugin for neovim with custom queries for tailwind class sorting without prettier. I am probably going to look into creating a small "Templ Attribute Sorter" neovim plugin using a similar method if I find time and there is any interest in the idea.

@jonerer
Copy link
Contributor Author

jonerer commented Jan 31, 2024

I'm very interested in trying to fit this into templ, as I enjoy the high power batteries-included experience provided by templ. It might be that it doesn't fit in templ, but I think it's hard to have an informed decision without trying it out.

I plan to add it in my fork behind a command line --sort-attributes and get a sense of it :)

@jonerer
Copy link
Contributor Author

jonerer commented Feb 20, 2024

So I implemented it and played around with it a bit. I think it's a nice improvement. It's not super revolutionary, but it's nice to always have the common attributes where you expect (class first). I'm using the "code guide" sorting, as can be seen in the PR.

However, the PR is not quite in a state where it could be merged. It works, but some comments and tests need updating. But for unrelated reasons I think I sadly won't find the time to finish it.

I don't know if anyone wants to take it over, or if I should just close it up for now? It doesn't seem to have that much momentum behind it anyway?

@RafaelZasas
Copy link

I left A Discussion post about possibly offloading the html formatting to the user in the way of some config file, or possibly with a prettier plugin. Prettier plugin for the std lib html/template package works great and allows me to configure exactly how the html should be formatted, while allowing the plugin to format the template handlebars interdependently.

I'm obviously happy with the go fmt being applied to the go code in the templ files, and I'm not opinionated about the script formatting (since I do not use it) but I personally love having html attributes on separate lines.

Having the order of the attributes standardized would be a nice benefit but I'm much more concerned with lines being longer than 80 chars and having to horizontally scroll the page just to see what the last few attributes on an input tag are.

@joerdav
Copy link
Collaborator

joerdav commented Feb 27, 2024

Similar to the prettier plugin for html/template, I don't believe that this should be something that comes out of the box or with templ, as it's quite opinion based so would require config, and supporting multiple behaviors for something like this could become complex, I'd ideally like to take the go fmt stance of having zero options.

It would be entirely possible for an external tool to import the templ parser but override some of the formatting responses :)

Though I think this conversation has ran it course @a-h @jonerer ? Feels like we're converging on this not being something we want to be the default, and we aren't looking to support formatting options at the moment.

Feel free to re-open if you think my closing of this ticket is pre-mature!

@joerdav joerdav closed this as completed Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fmt NeedsDecision Issue needs some more discussion so a decision can be made
Projects
None yet
Development

No branches or pull requests

5 participants