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

Hanging attributes #55

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

AndrewRadev
Copy link
Contributor

This PR implements a "hanging" attribute style. If the first line of the component has at least one attribute set on that line, the rest align according to that. So, it looks like this:

{{foo-bar one="two"
          two="three"
          four="five"}}

If the first line only has the component name, and not an attribute, it indents like it did before.

Unfortunately, I branched this PR off from my own master, which includes PRs #53 and #52. Depending on what you decide, I can rebase this on top of your master, but for now, I'll just leave it here for consideration as-is.

@nelstrom
Copy link

I'd love to see this merged. In the meantime, I've switched to using AndrewRadev/vim-mustache-handlebars.

@shadoath
Copy link

Bump. Is this repo being actively maintained?

@jsit
Copy link
Contributor

jsit commented Jul 16, 2019

@AndrewRadev Can you please resolve these conflicts?

@AndrewRadev
Copy link
Contributor Author

I've rebased the branch on top of the current master (or rather, cherry-picked the changes without the variable renames), and I've fixed a bug I'd introduced. I'd recommend testing it out before merging, though, indentation is tricky.

If later logic wants to jump around the buffer, we should keep the
cursor where it was.
* master:
  Support inverted components
  Instead of s:in_block_expr, search() for opening brackets
  Support .hb file extension
* master:
  Fix broken highlighting of hash before `unless`
  Fix word boundary match error
  Use word boundaries in conditionals and helpers
  Add 'let' helper
@AndrewRadev
Copy link
Contributor Author

I've merged the most recent master, and tested it out a bit, and it seems to work well now after #93. If you have time to test it out and find any issues to fix, let me know.

@jsit
Copy link
Contributor

jsit commented Aug 8, 2020

Thanks @AndrewRadev. Can you please add some test cases to example.mustache?

@AndrewRadev
Copy link
Contributor Author

@jsit I've pushed an example of component with hanging attributes, and I've also applied a fix for that file -- it had an unclosed <a> tag.

That said, I see there's a new example there of an angle-brackets component that doesn't seem to indent correctly:

<AngleBracketComponent
@mustache={{@bla}}
@number=123
data-test-id="test"
class="my-class" as |test|>
Some Text
<div>{{test.someValue}}</div>
<test.component teste="asdfads"></test.component>
</AngleBraketComponent>

But actually, even when I change to master, it still doesn't indent correctly, so that doesn't seem to be a regression from this PR. It might be something that's just not handled on my config and expects maybe HTML support to deal with it.

Well, either way, it seems like an unrelated issue.

@jsit
Copy link
Contributor

jsit commented Aug 9, 2020

@AndrewRadev Thanks; I've already got an issue for the angle bracket indent issue, #92

Will have a look at your hanging example now, thank you!

@jsit
Copy link
Contributor

jsit commented Aug 9, 2020

@AndrewRadev Also curious to get your opinion on #98

indent/handlebars.vim Outdated Show resolved Hide resolved
indent/handlebars.vim Outdated Show resolved Hide resolved
indent/handlebars.vim Outdated Show resolved Hide resolved
@AndrewRadev
Copy link
Contributor Author

I think I've addressed your comments -- let me know what you think.

@AndrewRadev Also curious to get your opinion on #98

Yes, that seems like a very sensible change. I remember being confused by the explicit runtimes of syntax and indent scripts, it would be much easier to understand to just have separate handlebars and mustache files where one includes the other.

endif
endif

" check for a closing }}, indent according to the opening one
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this part of hanging indent, too? Or is this a separate improvement? I just want to keep this PR as clean as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see now that this is necessary for the hanging indent. However, it looks like it duplicates some other functionality.

I notice that on this line:

https://github.com/mustache/vim-mustache-handlebars/pull/55/files#diff-eeee5d46b419bb17834d0b1a86a43f8fR114

It's returning indent(line) + sw, which is also happening later:

  " indent after block: {{#block, {{^block
  if prevLine =~# '\v\s*\{\{[#^].*\s*'
    let ind = ind + sw
  endif

Would it be possible to restrict this section to just the "hanging indent"-related conditionals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't the same case, though. The case that was already there applies on prevLine, while this new case is about the opening line of the prevLine.

Basically, the one that maches prevLine (the existing code) works for this case:

{{#foo}}
  [Cursor]
{{/foo}}

While the new code works on this case:

{{#foo one=two
       three=four}}
  [Cursor]
{{/foo}}

The previous line in this case is not the block opener, it's the three=four}} line. What's the indent on the next line from this one? Well, we need to find its opening line, which is the {{#foo line and take that one + one shiftwidth (if it's a block opener).

For a different explanation, consider the comments -- the existing code is indent after block: {{#block, {{^block. The new code is not indenting after a block tag, it's indenting after the closing part of any tag.

The only way I can see to unify them is to extend the new code to handle the old code's case, because "find the opening part" is the more generic logic. But trying it out, I seem to be breaking the {{else if case in the example, and I don't know why.

To be honest, I don't think it's worth the risk to try to combine the two cases, especially since it would make the one merged case more complicated and possibly trickier to change in the future. Let me know what you think and whether I've explained the difference between the cases well enough.

Comment on lines +112 to +117
if strpart(getline(line), col - 1, 3) =~ '{{[#^]'
" then it's a block component, indent a shiftwidth more
return indent(line) + sw
else
return indent(line)
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if strpart(getline(line), col - 1, 3) =~ '{{[#^]'
" then it's a block component, indent a shiftwidth more
return indent(line) + sw
else
return indent(line)
endif
if strpart(getline(line), col - 1, 3) !=~ '{{[#^]'
return indent(line)
endif

We could almost do this, see what I mean? the indent(line) + sw for "normal" indents is happening later anyway, so it would be nice to keep that there.

@jsit
Copy link
Contributor

jsit commented Aug 9, 2020

An issue here: if I'm typing out a mustache element, rather than indenting one with =, it stops correctly doing the hanging indent after the second line:

Screen Shot 2020-08-09 at 5 26 13 PM

Although this seems to be an issue on master, too, with standard blocks:

Screen Shot 2020-08-09 at 5 28 42 PM

Edit: Filed #99 for this unrelated issue.

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.

None yet

4 participants