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

Remote cell #328

Merged
merged 11 commits into from
Sep 4, 2023
Merged

Conversation

cristineguadelupe
Copy link
Contributor

Hey @jonatanklosko! @josevalim told me to ask you if there's a better way to give feedback on invalid codes.

@jonatanklosko
Copy link
Member

We can define scan_eval_result/2 and react to evaluation error any way we want. @josevalim is it for arbitrary errors, or do you have something specific in mind?

@josevalim
Copy link
Contributor

@jonatanklosko it is about the case where you have invalid Elixir code in the editor. Then Code.string_to_quoted will fail. I suggest to emit something like, so once evaluated it gives you the proper error, as you can see in this PR:

Code.string_to_quoted!("invalid code here")

Any other suggestions? Is there a way to attach code comments to the generated code?

@cristineguadelupe
Copy link
Contributor Author

We can define scan_eval_result/2 and react to evaluation error any way we want. @josevalim is it for arbitrary errors, or do you have something specific in mind?

Wait! I think I was a bit vague. 🙈
The problem is to provide feedback before evaluating. While typing the code and without crashing the SmartCell

@jonatanklosko
Copy link
Member

@josevalim perhaps it would be less confusing if we generated code with explicit raise "..."?

Is there a way to attach code comments to the generated code?

def to_source(attrs) returns string, so it can be anything we want.

@josevalim
Copy link
Contributor

@jonatanklosko the problem is raising the explicit syntax error that we get. What do you think about generating this:

# Invalid code for RPC, reproducing the error below
Code.string_to_quoted!(...)

?

@jonatanklosko
Copy link
Member

Sounds good!

node = unquote(String.to_atom(node))
cookie = unquote(String.to_atom(cookie))
Node.set_cookie(node, cookie)
Node.connect(node)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we actually don't need this call. :erpc connects automatically! Please verify:

Suggested change
Node.connect(node)

Comment on lines 57 to 58
node = unquote(String.to_atom(node))
cookie = unquote(String.to_atom(cookie))
Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, we should allow the cookie to be a secret, but we can move that to a separate issue. :)


def to_source(%{"code" => code} = attrs) do
code = Code.string_to_quoted(code)
to_quoted(attrs, code)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
to_quoted(attrs, code)
to_source(attrs, code)

quote do
node = unquote(String.to_atom(node))
cookie = unquote(String.to_atom(cookie))
Node.set_cookie(node, cookie)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: should we interpolate the cookie directly here?

Suggested change
Node.set_cookie(node, cookie)
Node.set_cookie(node, unquote(String.to_atom(cookie)))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that if we do it for one, we should do it for both

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so because node we repeat (we use it more than once). We don't do so for cookie!

Comment on lines 81 to 82
/>
</form>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/>
</form>
/>
</div>
</form>

@cristineguadelupe cristineguadelupe merged commit d9a0285 into livebook-dev:main Sep 4, 2023
1 check passed
@cristineguadelupe cristineguadelupe deleted the cg-remote-cell branch September 4, 2023 14:33
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