-
Notifications
You must be signed in to change notification settings - Fork 423
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
Add support for Erlang code cells #1892
Conversation
I actually like this approach a lot. It is a lightweight introduction of Erlang features and keeps compatibility with smart cells and everything else. The camelize/underscore is a bit unfortunate but also probably the best we can do here. We probably should also change the pretty printing to be Erlang based. We also need a visual indicator somewhere in the cell which reveals the language being used. Potentially in the top right corner of the cell but that space does not exist for one-liners. I think this could be enough for an initial version (unless @jonatanklosko has other thoughts) and then we can work on intellisense next. |
Yeah, I willfully ignored the documentation here because I was too lazy to write this code myself ;). There is no unsafety here (I think) because Erlang is not able to update entries (could be added with a local function, like
Two questions:
|
17695dd
to
73fe0e8
Compare
FWIW, they didn't include Elixir either but they did accept a PR. |
Uffizzi Preview |
There doesn't seem to be a Monarch grammar, yet. I'll see if I can whip something up (unless you are planning to switch to Treesitter or Textmate grammars eventually :)). |
No plans to swap at the moment. :) So I believe a Monaco grammar will be needed then! |
Hey, thanks for the PR! Here's a list of things that we need for an initial version:
Then we just need a few UI adjustments and I can handle those. We can have an arrow next to the With those changes in place it should be good to go and we can expand on intellisense features separately. |
|
|
Thanks for working on this, this is quite exciting. It would be awesome to see Erlang support in livebook |
Regarding tracing: This is a bit more complicated to do than the rest as Erlang's evaluation doesn't have a notion of tracing, so I'd either have to fake trace events after inspecting the AST, or adjust this whole code path a bit. |
You don’t need to do tracing, only reproduce its results. Since Erlang does not have aliases and imports, we only need to know the variables read by Erlang, which is equivalent to traversing its AST blindly and collecting all names from Note to self: we also need to make sure the first setup cell is always in Elixir. |
I added a very simple token-based tracing now. It's not actually correct, since it can't handle cases like Cell 1message = "test" Cell 2F = fun(Message) ->
Message
end,
F(1). (it will think that the Cell 2 depends on Cell 1, even though it doesn't). Fixing this would require actually visiting the whole AST which seems like overkill at this stage :) |
The only part missing is translation of the lexer/parser/interpreter errors, I'll do that today. I think I handled everything that @jonatanklosko suggested. |
@filmor the most important is the example where: "if cell 1 defines X, cell 2 defines Y, and cell 3 does X + Y, modifying cell 1 only marks cell 3 as stale". False positives because of anonymous function shadowing is fine IMO. |
Working on this a bit more, I think I've understood the variable tracking now :) |
Strike that, seems to be working fine. Anything else you'd want to see handled before merging this? |
172356e
to
9b3bcfe
Compare
Co-authored-by: José Valim <jose.valim@gmail.com>
(I plan to contribute more to LiveBook in the future, it would be helpful if I could always get CI feedback). |
(@filmor CI should be enabled for subsequent PRs, the manual approval is just in case for first-time contributions :) |
FTR waiting for the next Elixir rc, so we can merge #1918. I will resolve the conflicts! |
@filmor I updated the PR, the only thing remaining is better error messages, especially syntax/compiler ones. For example: This makes sense in |
@filmor I'm gonna merge this PR, feel free to open next one for the errors and then for intellisense features if you want :) |
Thank you so much for @filmor! To clarify about exceptions, the issue is here: If you return {:error, anything, stack}, if
Option 2 is better but Option 1 will be enough in the short term. The other thing is that, if the parser has an incomplete expression, it returns a particular error tuple to say so. This is used by the shell, for example, to know the expression is not complete and prompt for a new line. In our case, we want to detect this case (I am almost sure it is when the token is an empty list) and say something like: "incomplete expression". I would say those are "must fix" before a release. :) Then there are a bunch of enhancements we could add:
But I think at this point we can open up the challenges to the Erlang community as well. Once again, thank you! ❤️ |
I'll try to improve the error handling. I don't want to inspect the error string to decide whether the expression is complete, but traversing it to check for completeness is something I have done before (just needs to be updated to take |
The Erlang shell already detects when it is incomplete and IIRC it is when the parser returns an empty list as the error token. We shouldn’t need to pre-parse it. But ideally, we would also print all errors like the Erlang shell does (and not as Elixir exceptions). |
Adds Erlang code cells and translates bindings (primitively).
I haven't been able to get webpack to actually include the Erlang syntax highlighting for Monaco (even though it's configured). I also haven't looked into integrating Intellisense, though I'd guess that it wouldn't be an enormous amount of work.
Touches on #190.