Skip to content

Avoid indentation errors after 'end'. #41

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

Closed

Conversation

tomterl
Copy link
Contributor

@tomterl tomterl commented Mar 10, 2014

This is, if not a fix, a workaround for half of #18. In my elixir files
this produces no indentation errors; maybe you can test this against
your setup and see if it doesn't raise any red flags for you.

I don't know enough smie (yet), to look after the statement indentation
error, that is the other error reported in #18.

tomterl added 2 commits March 10, 2014 12:09
This is, if not a fix, a workaround for half of elixir-editors#18. In my elixir files
this produces no indentation errors; maybe you can test this against
your setup and see if it doesn't raise any red flags for you.

I don't know enough smie (yet), to look after the statement indentation
error, that is the other error reported in elixir-editors#18.
@tomterl
Copy link
Contributor Author

tomterl commented Mar 10, 2014

I'm currently digging into smie and elixir-smie.el to get the second problem solved (wrong indentation after virtual ; ).

The solution with :after . "end" doesn't look totally silly anymore after a look at the smie manual;

@tonini I saw that there are a couple of test cases regarding indentation, but a target test is not defined in the rakefile -- would a maintainer who knows about the tests be able to add a test target to the rakefile, so that I could use canonical tests for my changes without having to dig into ert ATM?

@antifuchs
Copy link
Contributor

Just sneaking in to say I love that you're taking this on, Tom! (:

I'm happy to walk you through running tests on emacs-elixir! They're ~undocumented at the moment (so sorry for that!). To run the full elixir-mode test suite, you can run elixir-mode-run-tests - it'll load the test files, and run the appropriate suite with ert. Here's the definition of the function, if you want to know the basic structure of what it does (:

@antifuchs
Copy link
Contributor

Also, I should add - feel free to make more tests (even if they're expected-failing atm)! The suite is very very basic, and the reason it doesn't catch most of the errors is that I just haven't written enough code with elixir-mode yet. S-:

@tomterl
Copy link
Contributor Author

tomterl commented Mar 10, 2014

Cool, Thanks tests are running (I had to add the emacs-elixir directory to load-path, other than that all set now.

More tests when I have something to test :)

@mattdeboard
Copy link
Contributor

So while I was messing with this same code region myself I found that replacing this line with the followingmade an additional test pass (up to 19 now, with the two I've added):

("def" non-block-expr "do" statements "end")

It also fixed the specific example of the problem described in #18 . However it's still broken if you try to add a declaration in module scope (use, require, alias, etc.). Don't know if this helps.

@mattdeboard
Copy link
Contributor

With that change and with a module-level declaration...

Good:

defmodule FooBar do
  use Baz
  def foo do
    if true, do: IO.puts "yay"
    20
  end

  def bar do
    if true, do: IO.puts "yay"
    20
  end
end

Bad:

defmodule FooBar do
  use Baz

  def foo do
    if true, do: IO.puts "yay"
    20
  end

    def bar do
      if true, do: IO.puts "yay"
      20
    end
end

And it's not just the blank line that is creating the issue. If I remove the declaration, it's really fine, even with extra blanks:

defmodule FooBar do

  def foo do
    if true, do: IO.puts "yay"
    20
  end

  def bar do
    if true, do: IO.puts "yay"
    20
  end
end

I suspect but don't know that use, require, import, and alias have a distinct syntax, a function call that's a sibling of a method definition. There's no end to a declaration. And since the declarations aren't defined in the grammar specifically there's a precedence conflict. Note here that the four declarations are basically meaningless to the grammar:

defmodule FooBar do
  garbage
  things
  that
  mean
  nothing


    def foo do
      if true, do: IO.puts "yay"
      20
    end

      def bar do
        if true, do: IO.puts "yay"
        20
      end
end

So maybe having like declarations being a distinct section of the grammar would help?

@mattdeboard
Copy link
Contributor

One final behavior:

defmodule FooBar do

  def foo do
    if true, do: IO.puts "yay"
    20
  end

  def bar do
    if true, do: IO.puts "yay"
    20
  end

    def ban do
      1 + 1
    end

end

So weird. I don't grok why the third method in line breaks indentation.

@tomterl
Copy link
Contributor Author

tomterl commented Mar 11, 2014

Cool.
I'll try to debug that further, As far as I can tell right now, the problem is that smie as setup handles lines it doesn't know the speficics about as hanging - the virtual ; doesn't really help, albeit it should somehow - that includes arithmetic expressions and use, etc. statements.

I'm in the process of groking the process and hope that solving/understanding the virtual ; handling will result in grammar/rules adjustments that make special handling of end and the likes of use unnecessary.

I so hope this isn't a wild goose chase :)

@tomterl
Copy link
Contributor Author

tomterl commented Mar 11, 2014

With the (:after . "end") rule in place, the indentation error after use etc isn't triggered.

The problem with indentation errors after newlines/comments is that consecutive newlines/comments aren't really ignored resp. treated as one - consider the following case

 a = 1 + 2
|b = 2 + 3

(| beeing the cursor ) the last token considered here is the injected ; as (:before . ";"), as it should and the indentation therefore is correct

 a = 1 + 2

              |b = 2 + 3

The last token considered here is basic (:elem . basic) with hanging:t - I'm trying to understand now, why the to \n characters aren't treated as one injected ;, as they should per my understanding.

Or am I going astray somehow (digging vertically in in something unknown leaves much out of focus :).

@mattdeboard
Copy link
Contributor

@tonini @antifuchs Are there any plans for progress on this PR? It's been almost 3 weeks and this syntactic breakage prevents me from using my primary editor for Elixir. I don't mind vim but obviously I'd greatly prefer to use Emacs.

@mattdeboard
Copy link
Contributor

Is there anything I can do to help progress on this PR? Please let me know, I will be quite happy to help.

@tomterl
Copy link
Contributor Author

tomterl commented Apr 29, 2014

I don't have time to play with elixir and the tools ATM. I'm pretty sure the problem is the implicit ';' -- the smie grammar looks correct, but nevertheless produces the described (#41 (comment)) discrepancy. If someone with a bit more knowledge could investigate that angle to see if it's nonsense or not, that would reduce the (perceived) costs for me to make time to look further into it.

@mattdeboard
Copy link
Contributor

@tomterl I understand. I have spent several evenings trying to get to the bottom of this issue myself. The code here looks like it fits the profile of these double-newline indentation errors, but I have a lot of trouble forming a good picture of the logic in my head.

I hope my previous comment didn't come across as nagging.

mattdeboard added a commit that referenced this pull request Jul 5, 2014
Add a SMIE rule function for "def". Fixes #38 and #41
@mattdeboard
Copy link
Contributor

Fixed in #48

@mattdeboard mattdeboard closed this Jul 5, 2014
J3RN pushed a commit to J3RN/emacs-elixir that referenced this pull request Apr 24, 2021
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.

3 participants