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

Demo Using CommonMark in Pluto.jl #956

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

jeremiahpslewis
Copy link
Contributor

@jeremiahpslewis jeremiahpslewis commented Feb 28, 2021

Demo for issue #957, showing how seamlessly CommonMark can be used with Pluto now that the package manager is implemented.

Three ways forward, think the first one is the way to go:

  1. As demonstrated in this PR, import CommonMark.jl using new package manager and use cm macro
  2. Import and reexport cm macro as part of Pluto
  3. Overload md macro with CommonMark.jl's cm macro and do 2.

@fonsp
Copy link
Owner

fonsp commented Mar 1, 2021

Hey! Thanks for helping with our markdown samples!

For this PR, I think that the complexity of installing and setting up commonmark does not belong in the "getting started" notebook. This notebook should not use any packages, and the Markdown formatting issues are very niche compared to the challenge of learning Julia and Pluto.

We could make a separate sample notebook about CommonMark, but the reader might wonder: "If this is such an essential package, then why does Pluto come with Markdown pre-imported, but not with Commonmark?" Which is a great question! What is the status of replacing Markdown with CommonMark in the julia stdlib?

If commonmark is the solution to markdown issues, then I hope that it becomes part of the Julia stdlib. Otherwise, we are replacing the headache of using Markdown with the headache of "boilerplate code to install commonmark".

@MichaelHatherly
Copy link
Contributor

What is the status of replacing Markdown with CommonMark in the julia stdlib?

In simple terms, it requires some time on my part (or someone willing to learn the CM.jl and MD.jl codebases) to make sure we have feature parity with MD.jl and then execute a transition phase away from it to CM.jl, which will be time consuming since their underlying datastructures are not similar and most heavy users of MD.jl are using it's internals.

Timeframe is "when it's ready" 😄

@jeremiahpslewis
Copy link
Contributor Author

jeremiahpslewis commented Mar 1, 2021

Marked this PR as a draft, idea is to track how close CommonMark.jl is to being a full replacement for Base.Markdown in the context of Pluto.jl's use case.

As for the headache of 'boilerplate code', my proposal would be (once CM.jl, MD.jl feature parity in context of Pluto.jl's use of the package is reached), to add CM.jl as a dependency (naturally, can already see reasons why you might have objections to this).

Importantly, however, while you are right that:

formatting issues are very niche compared to the challenge of learning Julia and Pluto.

nonetheless, these issues are the difference for many users between a 'just works' experience where LaTeX, interpolation and markdown can be used together harmoniously in notebooks and an experience where there are many mysterious formatting issues. I tried to highlight this by linking to Github Issues already posted on this topic, would be interested to hear whether you can see a trend in the anonymous feedback as to whether this is a recurrent issue. :)

@jeremiahpslewis
Copy link
Contributor Author

I think that the proposal in JuliaLang/julia#37337 is quite coherent in putting forward the idea of different ideas of 'feature parity', depending on the use case / application. In this context, it would seem like Pluto.jl could be satisfied by Markdown feature parity much sooner than a library using Markdown.jl object manipulations.

@fonsp
Copy link
Owner

fonsp commented Mar 1, 2021

This all sounds great, thanks for your work! I definitely support it from the Pluto side.

Some notes:

  • Adding CM as a dependency is difficult right now, because the dependencies of Pluto.jl as a notebook server and the dependencies of your notebook code are currently intertwined in a strange way. (Long story short: notebooks only have inherent dependencies on stdlibs: Markdown and InteractiveUtils, to avoid problems). But this could be a possibility after 🎁 Built-in Pkg management #844
  • Move to backtick latex delimiting #953 is a good initiative, and we could use syntax highlighting to warn users about $-latex during the transition
  • Pluto does do some special treatment for Markdown, besides importing it by default. Mainly
    # this code block will run cells that only contain text offline, i.e. on the server process, before doing anything else
    # this makes the notebook load a lot faster - the front-end does not have to wait for each output, and perform costly reflows whenever one updates
    # "A Workspace on the main process, used to prerender markdown before starting a notebook process for speedy UI."
    original_pwd = pwd()
    offline_workspace = WorkspaceManager.make_workspace(
    (ServerSession(options=Configuration.Options(evaluation=Configuration.EvaluationOptions(workspace_use_distributed=false))),
    notebook,)
    )
    to_run_offline = filter(c -> !c.running && is_just_text(new, c) && is_just_text(old, c), cells)
    for cell in to_run_offline
    run_single!(offline_workspace, cell, new[cell])
    end
    and macro expansion to detect interpolated variables:
    const can_macroexpand_no_bind = Set(Symbol.(["@md_str", "Markdown.@md_str", "@gensym", "Base.@gensym", "@kwdef", "Base.@kwdef", "@enum", "Base.@enum"]))
    const can_macroexpand = can_macroexpand_no_bind Set(Symbol.(["@bind", "PlutoRunner.@bind"]))
    macro_kwargs_as_kw(ex::Expr) = Expr(:macrocall, ex.args[1:3]..., assign_to_kw.(ex.args[4:end])...)
    """
    If the macro is known to Pluto, expand or 'mock expand' it, if not, return the expression.
    . But this does not depend on Markdown's internal structure.

@jeremiahpslewis
Copy link
Contributor Author

@fonsp quick update, thanks to @MichaelHatherly CommonMark.jl now has a branch which fully supports a markdown macro including interpolation (but which does not, by default, support dollar math), I've updated this PR to direct import the package in case you're curious

@fonsp
Copy link
Owner

fonsp commented Mar 10, 2021

That's awesome! I think that disabling dollar math is a good choice.

I am excited for CommonMark.jl, but I won't be able to follow its development very closely. Right now I think the best way forward is to wait for #844 and maybe reconsider using CommonMark as a default package, but ideally I would like to see it implemented in the stdlib. Tag @fonsp if you need me in the meantime!

@jeremiahpslewis
Copy link
Contributor Author

Sounds good. Ping me when #844 is done (at which point I assume the macro will be available on a released version of CommonMark) and I'll put together a Pluto.jl notebook demonstrating the advantages.

Base automatically changed from master to main March 11, 2021 21:33
@fonsp fonsp added documentation display & PlutoRunner & AbstractPlutoDingetjes.jl labels Apr 7, 2021
@DhruvaSambrani
Copy link
Contributor

DhruvaSambrani commented Jul 10, 2021

@jeremiahpslewis , a reminder that #844 is merged and released

@jeremiahpslewis
Copy link
Contributor Author

Thanks!

@jeremiahpslewis jeremiahpslewis marked this pull request as ready for review July 13, 2021 14:15
@jeremiahpslewis
Copy link
Contributor Author

@DhruvaSambrani @MichaelHatherly think this is now ready for consideration; really blown away by the new package manager in Pluto.jl, feels like magic to use, but still miraculously produces intuitive boilerplate and commits. 🦄

sample/Getting started.jl Show resolved Hide resolved
sample/Getting started.jl Show resolved Hide resolved
sample/Getting started.jl Show resolved Hide resolved
sample/Getting started.jl Show resolved Hide resolved
sample/Getting started.jl Show resolved Hide resolved
@jeremiahpslewis
Copy link
Contributor Author

@DhruvaSambrani think you may have seen an older diff, all the md macros were covered in my commit 3046d30

@DhruvaSambrani
Copy link
Contributor

Yup, there are some HTML macros left though

@jeremiahpslewis
Copy link
Contributor Author

jeremiahpslewis commented Jul 13, 2021

@DhruvaSambrani Not sure whether it really makes sense to use cm markdown macros for raw html. What is your reasoning for wanting to replace them?

@DhruvaSambrani
Copy link
Contributor

Ah ok,
So my thinking was that the only use of HTML in this notebook is to add attributes to an image. But commonmark allows us to add attributes and I felt that we should illustrate that in this notebook too!

@MichaelHatherly
Copy link
Contributor

to add attributes to an image

cm" should support those fine, though it's never going to manage to be 100% successful with all embedded HTML from what I understand of the spec. I think it's probably worth at least trying to use cm" where possible, but falling back to html" for the more complex cases.

@jeremiahpslewis
Copy link
Contributor Author

@DhruvaSambrani @MichaelHatherly Ok, so I tested replacing the html macro as well as the HTML( function, the former works with cm and has been swapped out, the latter is going to need to stay as-is with the function call.

@DhruvaSambrani
Copy link
Contributor

DhruvaSambrani commented Jul 13, 2021

cm"""**Well done, your cat is called $(cat) now.** This text gets updated every time you change the name. To see how the magic works, click on the ![eye](https://cdn.jsdelivr.net/gh/ionic-team/ionicons@5.5.1/src/svg/eye-outline.svg){ width: 1em, height: 1em, margin-bottom: -.2em }to the left of this text."""

Doesn't this work?

@jeremiahpslewis
Copy link
Contributor Author

Syntax is fine, rendering fails to interpolate $(cat) to cat afaict.

@MichaelHatherly
Copy link
Contributor

MichaelHatherly commented Jul 13, 2021

Syntax is fine, rendering fails to interpolate $(cat) to cat afaict.

Is it specific to Pluto, or is it also happening in the REPL?

@MichaelHatherly
Copy link
Contributor

MichaelHatherly commented Jul 13, 2021

Is it not just because #1032 is not implemented yet?

@jeremiahpslewis
Copy link
Contributor Author

jeremiahpslewis commented Jul 14, 2021

@MichaelHatherly Tried running it in the repl, can confirm that this issue is reproducible outside of Pluto. See MichaelHatherly/CommonMark.jl#22 which I just created for documentation, the second level tag seems to break things.

@jeremiahpslewis
Copy link
Contributor Author

@MichaelHatherly @DhruvaSambrani after another round of feedback, think we've reached a steady state: because of CommonMark spec (and reasonable idea of separation of concerns), let's keep html snippets in html, otherwise we're going to start encountering the same unpredictable behavior in CM.jl that we are trying to avoid from MD.jl. If you want good, no headaches markdown, then use the cm macro, if you want html, then go for the existing Julia html utilities, they work well with no hassles. I've added a 'doc' snippet to issue MichaelHatherly/CommonMark.jl#22 to explain this, can imagine something similar might belong in doc pages to CM.jl when it's time to staff up that part of the package.

@DhruvaSambrani
Copy link
Contributor

DhruvaSambrani commented Jul 14, 2021

While the CommonMark issue is a bug that needs to be fixed, why does it affect our example? The <p/> tags are not necessary, as they should be appended while displaying.

@MichaelHatherly
Copy link
Contributor

While the CommonMark issue is a bug that needs to be fixed, why does it affect our example? The

tags are not necessary, as they should be appended while displaying.

Yes, removing the p tags is enough to allow the interpolation in this case since p and b are in different HTML "types" as defined by the commonmark spec. p is a block of HTML whereas b is just a single tag.

Unless the commonmark spec changes there'll be no changes to the parsing precedence in CommonMark.jl for this case. You're welcome to look into a some kind of fix to make it work, but it needs to fit in with the spec.

@jeremiahpslewis
Copy link
Contributor Author

jeremiahpslewis commented Jul 14, 2021

@DhruvaSambrani Think @MichaelHatherly has a good point here that CommonMark.jl is spec driven, we're not talking about a bug.

Not sure I clearly expressed my other concern: why push html into markdown when we can just use html, e.g. if not broken, don't fix it; html function allows for full expressiveness of html; less of a question of what we can force to work for the minimal demo script.

@fonsp
Copy link
Owner

fonsp commented Dec 16, 2021

Sorry for leaving this PR for so long! I took at look at it again today, and this led to a fun experiment! MichaelHatherly/CommonMark.jl#31

Since we have #844 , it is now much easier to use packages inside sample notebooks, and the plan is to add lots and lots of new sample notebooks during the next months! We should definitely do one that uses CommonMark, either directly, or using the new CommonMark macro if MichaelHatherly/CommonMark.jl#31 turns out to work as good as it seems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display & PlutoRunner & AbstractPlutoDingetjes.jl documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants