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

Add user widgets tutorial #1407

Merged
merged 10 commits into from
Aug 6, 2022
Merged

Add user widgets tutorial #1407

merged 10 commits into from
Aug 6, 2022

Conversation

Vtec234
Copy link
Member

@Vtec234 Vtec234 commented Aug 2, 2022

Adds a tutorial for user widgets. It is placed in doc/examples/ because that folder seems to get special treatment with Alectryon and I didn't want to otherwise mess with the build setup.

Also adds some helpers for RequestM operations.

Also adds the include_str macro as pretty much every user widget needs it, and it seems generally useful.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2022

Thanks for your contribution! Please make sure to follow our Commit Convention.

@Vtec234
Copy link
Member Author

Vtec234 commented Aug 2, 2022

1354
[00:49 (00.000514)] lean-doc-test> /build/mdbook-EX1bMe/widgets.md_3.lean:130:0: error: unterminated comment

It looks like triple-backtick blocks are possibly prevented within doc comments. However I need this one since it's JS, not Lean.

@EdAyers EdAyers mentioned this pull request Aug 3, 2022
1 task
EdAyers referenced this pull request Aug 3, 2022
Copy link
Contributor

@EdAyers EdAyers left a comment

Choose a reason for hiding this comment

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

It's fantastic

@EdAyers
Copy link
Contributor

EdAyers commented Aug 3, 2022

Maybe one thing we could sneak in to this PR is an include_str macro?

doc/widgets.lean Outdated Show resolved Hide resolved
@EdAyers
Copy link
Contributor

EdAyers commented Aug 5, 2022

It looks like markdown code blocks within /-! comments are not being translated properly (regardless of lean vs lean,ignore vs javascript header). Running mdbook build on such a file produces messed up html. Can we delete the javascript code block for now (the javascript code is still present in the string); and then open a new issue to support code blocks?

Copy link
Member

@Kha Kha left a comment

Choose a reason for hiding this comment

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

Great read!

of a widget instance as an instruction for the infoview: "when the user places their cursor here,
please render the following widget".

Every widget instance also contains a `props : Json` value. This value is passed as an argument
Copy link
Member

Choose a reason for hiding this comment

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

"contains" is a bit vague here. Could you say more about the value's lifetime regarding server and client?

Copy link
Member Author

Choose a reason for hiding this comment

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

I left that in but clarified widget instances a little above, is it better? In this case contains just means that wherever a widget instance is stored, we also store props.

Copy link
Member

Choose a reason for hiding this comment

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

Hah, now I understand why it's a JSON

doc/examples/widgets.lean Outdated Show resolved Hide resolved
doc/examples/widgets.lean Outdated Show resolved Hide resolved
src/Lean/Elab/IncludeStr.lean Outdated Show resolved Hide resolved
src/Lean/Server/Requests.lean Outdated Show resolved Hide resolved
@@ -65,31 +63,42 @@ structure RequestContext where
initParams : Lsp.InitializeParams

abbrev RequestTask α := Task (Except RequestError α)
abbrev RequestT m := ReaderT RequestContext <| ExceptT RequestError m
Copy link
Member

Choose a reason for hiding this comment

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

Is this all just for being able to call readDoc inside of run*M instead of outside?

Copy link
Member Author

@Vtec234 Vtec234 Aug 5, 2022

Choose a reason for hiding this comment

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

It's mostly so that we can type the argument to runM as RequestT M. Consequently the code inside can still access the request handler context, and throw request errors rather than only a generic IO.Error. Another way to put it is that it's kind of commuting the request and termelab/meta/command effects.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, it's about exceptions. As you may know I'd like to avoid duplicate exception layers, but I guess that's not realistic here. What error codes other than internalError are you expecting widget handlers to use?

Copy link
Member Author

Choose a reason for hiding this comment

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

At least invalidParams for incorrect arguments.

Copy link
Member

Choose a reason for hiding this comment

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

I had hoped that would be covered by the deserialization, but I guess the param type will not necessarily describe all argument constraints. I'm still not sure if it's worth the (not terribly big) code complication. The RPC interface of a widget should probably be considered private to the widget, so it's unlikely the client code will expect and handle invalidParams specially, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

I could definitely see people writing widget-support RPC libraries which can be used by multiple JS codebases, in which case some kind of error handling needs to happen. For example, our own getWidgetSource is now a bit simpler since it can run in CoreM and use all the standard functions rather than accessing parts of the Snapshot data.

src/Lean/Server/Requests.lean Outdated Show resolved Hide resolved
@Vtec234 Vtec234 requested a review from Kha August 5, 2022 20:34
@leodemoura leodemoura merged commit 962a4bf into leanprover:master Aug 6, 2022
@Vtec234 Vtec234 deleted the widget-doc branch August 6, 2022 19:34
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.

5 participants