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

[WIP] Add keyword pairs for ocaml #886

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Khady
Copy link
Contributor

@Khady Khady commented May 24, 2018

Also remove the pair of back quotes as is it used for polymorphic variants.

The pair let in is missing, because in is optional when let is used at toplevel and I don't know how to express that.

@Fuco1
Copy link
Owner

Fuco1 commented May 24, 2018

What does it mean that the let is top-level? If there is a predicate we could write we can then add it to the :unless clause so that it will stop the pairing. But if it is difficult to compute then we can leave it be.

@Khady
Copy link
Contributor Author

Khady commented May 24, 2018

Toplevel means it is at the root of a module, not in a function or a another let.
Maybe it is possible to have a rule based on the indentation for the let that are not in module M. I don't have good ideas for the ones in the module M part.

(* this is toplevel *)
let v = 1

(* this is also toplevel *)
let f a b =
  (* this is not toplevel *)
  let c = a + b in
  print_int c

(* toplevel *)
let v =
  (* not toplevel *)
  let a = 1 in
  a + 1

module M = struct
  (* this is toplevel *)
  let v = 1

  (* this is also toplevel *)
  let f a b =
    (* this is not toplevel *)
    let c = a + b in
    print_int c

  (* toplevel *)
  let v =
    (* not toplevel *)
    let a = 1 in
    a + 1
end

@Khady
Copy link
Contributor Author

Khady commented May 24, 2018

3 unexpected results:
   FAILED  sp-test-crystal-slurp-backward
   FAILED  sp-test-crystal-slurp-forward
   FAILED  sp-test-elixir-do-block-insertion-in-string

If I understand correctly, the build is not failing because of my changes, right?

@Fuco1
Copy link
Owner

Fuco1 commented May 24, 2018

About the build, probably not. Things sometimes break as we always pull the latest versions of other packages so they might have introduced some changes.

About the top-level let, I see. I think indentation is a bit dangerous if ocaml is not indentation-driven. I don't know ocaml, but say in Haskell (or even Python) this would work as the blocks there are handled by indent.

If module/end is a pair we can ask for (sp-get-enclosing-pair) and then if it is module that would mean we are at the "top level inside module", because it is the directly enclosing pair. However calling this in a context where there is no parent can get extremly slow as we will end up scanning the entire buffer and then figure out "oh well, no parent there after all /shrug". So for the let-check this might be debilitatingly slow.

@Khady
Copy link
Contributor Author

Khady commented May 24, 2018

ocaml is indeed not indentation-driven.

I can give a try to the (sp-get-enclosing-pair) idea. But probably not right now. So I guess it can be another PR.

@Khady
Copy link
Contributor Author

Khady commented May 24, 2018

One issue with current implementation is missing space during slurping. With | being the point, execute sp-forward-slurp-sexp.

module M1 = struct

end

module M2 = struct

end

It gives this code, with a space missing between module and end

module M1 = struct 



  moduleend M2 = struct

end```

and when it happens I can't slurp anymore. Not sure how to fix that properly.

@Fuco1
Copy link
Owner

Fuco1 commented May 24, 2018

Right. You can look into the ruby/elixir configurations, there are some examples, though for ruby it is a bit too complicated because of many optional features.

In short what you can do is you write a function and place it either on the :pre-handlers or :post-handlers and listen on the slurp(barf)-forward(backward) actions. The main handler for this is in sp-ruby-pre-handler for example. There you can insert spaces or newlines to make the new code valid.

@Fuco1
Copy link
Owner

Fuco1 commented May 24, 2018

Though actually, in your example the end should jump after the end of the second module not into the module as that actually invalidates the structure. You should probably add a module ... end pair also.

Edit: nevermind, it's the struct .. end that defines the scope. Disregard this comment.

@Khady Khady changed the title Add keyword pairs for ocaml [WIP] Add keyword pairs for ocaml May 27, 2018
@Khady
Copy link
Contributor Author

Khady commented Jun 17, 2018

I didn't have time to manage to make the keywords work properly yet. So I extracted the removal of quote and back quote to the PR #900 as they force me to disable strict mode in ocaml currently. Will try again to work on the keywords, but it shouldn't block the other PR.

Note that in elixir their is the same problem of missing space than what I have.

Before sp-forward-slurp-sexp:

defmodule ModuleName do
  def hello do
    IO.puts "Hello World"<|>
  end
end

defmodule ModuleName do
  def hello do
    IO.puts "Hello World"
  end
end

After:

defmodule ModuleName do
  def hello do
    IO.puts "Hello World"<|>
  end


  defmodule ModuleName do
    def hello do
      IO.puts "Hello World"
    end
    endend

@Khady
Copy link
Contributor Author

Khady commented Aug 11, 2018

I read a comment saying that for ruby smartparens would outsource the parsing the ruby-mode which is based on SMIE. Has it been done? Tuareg for ocaml is also based on SMIE. So ocaml support in smartparens could use the same technique if ruby-mode parser is used.

http://emacsredux.com/blog/2013/12/31/a-peek-at-emacs-24-dot-4-smarter-show-paren-mode/#comment-1182307181

@Fuco1
Copy link
Owner

Fuco1 commented Aug 11, 2018

Nope, haven't been done yet.

@Khady
Copy link
Contributor Author

Khady commented Aug 12, 2018

I added a rule to try to support {||} strings, but this can't be validated until #919 is fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

2 participants