Auto-indent embedded views

[sodapopcan]

I'm submitting this PR as a request for feedback. It aims to solve the indentation issues with embedded views (stuff between ~L, ~H, ~e, and ~E) as per #536. I couldn't figure out how to embed indent syntax—and assumed this is probably not possible—so this is what I've come up with. The aim is to provide an improved experience over the current behaviour, not to write a full re-implementation HTML indentation.

First and foremost, while I've added some test cases, I cannot for the life of me figure out how to run them. $ bundle exec parallel_rspec spec has every test failing with Vimrunner::NoSuitableVimError. bin/vim boots up a vim instance but I'm not sure how to target it.

Otherwise, there are a couple of outstanding issues I'm aware of:

• There is some funky behaviour when using vim-surround on a multi-line opening tag, though this seems to be current behaviour of the plugin.
• close-tag's > insert mapping leaves the cursor at the same indentation level as the opening tag.
• /> on a line by itself is caught by the top_of_file handler. Moving my proposed handler to the opening position fixes this, but I'm currently worried about doing so since I can't run the tests.

Thank you!

[jbodah]

I couldn't figure out how to embed indent syntax—and assumed this is probably not possible

vim-ruby's ERB has some examples of this. https://github.com/vim-ruby/vim-ruby/blob/4788a08433c3c90e131fc7d110d82577e1234a86/indent/eruby.vim#L36 is where the new indentexpr is defined for ERB, and it is composing the HTML and Ruby indentation expressions. It seems a little awkard however that we might possibly have a cycle here (Elixir embeds EEx which embeds Elixir)

As long as we have tests for anything we implement, it doesn't really matter that much to me what the implementation of this looks like (assuming we can't further write breaking tests), so doing it this way is better than being unsupported altogether, and tests should keep us from breaking users in the future

First and foremost, while I've added some test cases, I cannot for the life of me figure out how to run them. $ bundle exec parallel_rspec spec has every test failing with Vimrunner::NoSuitableVimError

I just updated the README with some additional information. The test suite depends on a GUI vim (gvim, mvim). The easiest solution is to have one of these in your PATH, however I also added a simple dotfile for specifying the path if the binary lives elsewhere. Please let me know if that works for you or not

Otherwise, there are a couple of outstanding issues I'm aware of

I'm not super familar with each of these - would you be able to take a snapshot of this behavior? If it isn't too serious then I think it's okay to merge even if these integrations are broken (I generally take the stance that we do not support integrations first-class so that we can get larger features like this merged). We can create follow-up issues for tracking if these are minor

[jbodah]

Just ran this locally - the tests are passing for me with the exception of the new tests. The i helper generates two test cases - one which tests that "select all-indent" works properly and another that tests indent is correct when typed. I think this will be fine if we remove the latter case

Here's an example test that only implements the first case:

vim-elixir/spec/indent/documentation_spec.rb

Lines 14 to 24 in 527e6fd

	it "bulk indenting doc blocks" do
	expect(<<~EOF).to be_elixir_indentation
	defmodule Test do
	@doc """
	do not reindent
	any indent that i do
	please
	"""
	end
	EOF
	end

[jbodah]

This moves the cursor - is this necessary? We do reset the cursor between handler calls so it's not a huge deal, but I'd be curious what this enables

[jbodah]

(Note this might be superseded by the syntax comment)

[jbodah]

We might be able to simplify this with a syntax check, e.g.

Given

def render(assigns) do
  ~H"""
  <Component
    id="some-id"
    value={{ @some_value}}
  >
    Inner HTML
  </Component>
  """
end

With synstack(3, 3) (note: cursor pos) we get: [145, 396, 163, 157]. Those can be mapped to the respective syntax group. synIDattr(396, "name") returns elixirSurfaceSigil. So we can dump the stack and search if we're nested in an embedded view this way

Here are the corresponding syntax expressions:

vim-elixir/syntax/elixir.vim

Lines 113 to 116 in 527e6fd

	syntax region elixirLiveViewSigil matchgroup=elixirSigilDelimiter keepend start=+\~L\z("""\)+ end=+^\s*\z1+ skip=+\\"+ contains=@HTML fold
	syntax region elixirSurfaceSigil matchgroup=elixirSigilDelimiter keepend start=+\~H\z("""\)+ end=+^\s*\z1+ skip=+\\"+ contains=@HTML fold
	syntax region elixirPhoenixESigil matchgroup=elixirSigilDelimiter keepend start=+\~E\z("""\)+ end=+^\s*\z1+ skip=+\\"+ contains=@HTML fold
	syntax region elixirPhoenixeSigil matchgroup=elixirSigilDelimiter keepend start=+\~e\z("""\)+ end=+^\s*\z1+ skip=+\\"+ contains=@HTML fold

The naming is a little awkward as the region isn't really the sigil - but it should still work I think

[jbodah]

Thanks for contributing this! It looks good. I'd like to give the actual indent rules a deeper read, but it looks correct so far

[sodapopcan]

Hi, thanks so much for the review and feedback!

I have now been able to get the tests running but it seems no matter what indentation I put, they pass.

  it "indents" do
    expect(<<~EOF).to be_eelixir_indentation do # doesn't matter if I use this or `be_elixir_indentation`
    ~e"""
    <div>
      <p>
        Some text
        </p>
          </div>
    """
    EOF
    end
  end

The above passes. Do you know why that would be?

Thanks for the tip about synstack()! I've refactored to use it. Previously, there was certainly no need to move the cursor, the n flag just slipped my mind.

In general, this was a pretty rushed PR. I decided I was going to solve this problem on Saturday and I got overly-excited when everything started working so I made a PR the same day. I would like to keep it as a draft for another week or so while I test it out and work out some kinks, and I'm hoping some others will try it as well. And yes, that'd be great if you could think on the rules. I've already made some changes that I haven't pushed yet but will do that at some point today.

Thanks again!

[sodapopcan]

I'm not super familar with each of these - would you be able to take a snapshot of this behavior? If it isn't too serious then I think it's okay to merge even if these integrations are broken (I generally take the stance that we do not support integrations first-class so that we can get larger features like this merged). We can create follow-up issues for tracking if these are minor

Close tag auto-closes html tags. If you type a second >, it will put them on separate lines with the cursor in betweem. The problem I'm seeing is fairly minor:

  <div>
  | <!-- cursor ends up here, inline with the tags instead of indented
  <div>

I can't even begin to describe what happens with vim-surround but suffice it to say it's an existing problem with multi-line opening tags in any markup file.

[sodapopcan]

Ok, I've pushed (I also intend to squash all these before requesting to merge).

I moved the embedded rules to the top. This solved a few issues I was having:

• Pressing <cr> in insert on after an opening sigil would make the sigil fly off to the right
• A solitary /> would end up heavily indented to the right (it was being caught by starts_with_binary_operator)
• ...this was yesterday and I can't remember the third thing 😬

I think in the end a few of the branches can be collapsed into one since they all have the same body, though it certainly is helpful having them separated for debug purposes.

[sodapopcan]

Also, off-topic question: I'm working on syntax highlighting for Surface—would that belong in this repo or its own?

[sodapopcan]

So, I restarted this because a whole whack of stuff wasn't working in my original PR. I've spent so much time on this over the past week that I still haven't had a lot of time to actually use it. I've added several test cases though the code itself is pretty hard to reason about. I realized there was no way I was going to get away with simply parsing HTML with searchpair() so it's turned quite complex. It could probably be cleaned up a bit and I'll be looking at that. The last thing I have to do is make sure all Surface directive stuff works. Otherwise, I just wanted to give an update and solicit some more feedback.

Even if this remains complex, I would love it if this could be merged and hopefully get more eyes on this. I will definitely be available to support as my care level that this works is very high :)

[jbodah]

Also, off-topic question: I'm working on syntax highlighting for Surface—would that belong in this repo or its own?

Here is fine. I've become more lax about including things outside of base Elixir now that this plugin is fairly stable

[sodapopcan]

I've pulled this out of draft mode and squashed into one commit. The logic is pretty complex and can probably be simplified but I just wanted some feedback and I'm trying to get people to try it out. I've been using it the past week and fixing things along the way. Other than the complexity, the rules seem pretty solid. I have some pretty decent tests so I can definitely try and simplify it, I'm just a little burnt out on it atm.

[vinniefranco]

@sodapopcan awesome work on this! hopefully it's merged in soon

[sodapopcan]

@vinniefranco Thank you, although I have not had time to work on this in quite some time. For a while there, I was spending all my hobby time on programming which led to some burn out. I've since been focusing on non-programming hobbies and since I don't write elixir professionally, I haven't had the motivation to get back to this. It's been simmering in the back of my mind but I have no idea when I'll be getting back to it. I feel my approach is a little brute force. It works quite well with Surface templates (and I think these style of templates are now in LiveView... am I mistaken?) but still has a few (perhaps severals) cases where it messes up standard leex templates. All this to say that I'm sorry, I don't know when this will be complete! It could be quite a while unless someone swoops in and takes over.

[nkezhaya]

@sodapopcan I rebased your branch off of the upstream master and can confirm it's still working really well!

[jbodah]

Tests are passing; Going to merge this - we can follow-up on issues via bug reports

[sodapopcan]

Great, thanks for merging! I'm actually just getting back into a LiveView project using HEEx so I'll be testing this out a bunch.