pragdave / earmark

Markdown parser for Elixir
Other
859 stars 135 forks source link

Parser situation is confusing #493

Closed martosaur closed 2 months ago

martosaur commented 3 months ago

I was doing a routine dependency update and found myself quite lost on what's the current situation on Earmark and Earmark parser. I will try to reconstruct what happened to help anyone on the same quest and potentially discuss what can be done to mitigate some further confusion.

  1. Earmark used to depend on EarmarkParser up until 1.4.40
  2. Earmark copied (?) some version (1.3.35?) of EarmarkParser code and dropped the dependency in version 1.4.41
  3. Versions 1.4.41 and 1.4.42 had some problems and were retired from Hex. 1.4.43 is the first available version that does not depend on EarmarkParser package.
  4. EarmarkParser module was renamed into Earmark.Parser in version 1.4.44
  5. Earmark have seen some minor changes and is now at version 1.4.46
  6. EarmarkParser package also seen some changes and is now at version 1.4.39
  7. Earmark got a new maintainer (Hi @amit-chaudhari1 👋 )

Unless I misunderstood something, this all means that:

  1. Earmark.Parser diverged from EarmarkParser development.
  2. Docs saying that AST generation has now been moved out to EarmarkParser aren't quite correct.
  3. There is no official, documented or straightforward way to plug EarmarkParser package into Earmark or to compose them. Earmark.as_html works on lines, not ast itself and uses Transform.postprocessed_ast and Transform.transform under the hood. It looks like we can just shove ast into Transform.transform, but it isn't obvious that would be equivalent.

Is this more or less correct? If so, there is probably a room for clarifying current state of things. I would contribute to the docs, but to be honest, I don't quite understand why the whole parser thing happened and I couldn't find any paper trail apart from this line in the changelog.

amit-chaudhari1 commented 2 months ago

Hello 👋,

Agree on the part that this is a bit confusing. Not sure on how I could clarify that as I'm not clear myself.

@RobertDober could maybe help us a bit here please?

@martosaur could ofc help as well...

RobertDober commented 2 months ago

Well the basic idea was:

Also I still hope that a PEG parser based EarmarkParser V2 will see the light one day

martosaur commented 2 months ago

Thanks for getting back to me!

Using EarmarkParser again in Earmark will not work as I am getting rid of all options that are not relevant for ex_doc

So the Earmark build-in parser will focus exclusively on "ExDoc flavoured markdown" so to say. This makes sense to me.

Does Earmark aim to support working with AST produced with EarmarkParser?

RobertDober commented 2 months ago

So the Earmark build-in parser will focus exclusively on "ExDoc flavoured markdown" so to say.

I would not call it built in, it is a seperate and independant package

martosaur commented 2 months ago

I would not call it built in, it is a seperate and independant package

Sorry, I was referring to Earmark.Parser built into Earmark package. And my follow up question is about EarmarkParser the separate package.

RobertDober commented 2 months ago

No problem @martosaur I will open an issue here for something that was reported to EarmarkParser and crashes on Earmark too. I will also open an issue in both projects that should explain things better.

martosaur commented 2 months ago

Thanks!

Just a few closing thoughts:

  1. if my understanding is correct, and Earmark.Parser is only meant to be used with ExDoc markdown, it might be helpful to rename it to ExDocParser or something like this.
  2. if EarmarkParser AST is meant to be compatible with Earmark, then it would be crucial to have Earmark.as_html(ast) function.
RobertDober commented 2 months ago

if my understanding is correct, and Earmark.Parser is only meant to be used with ExDoc markdown, it might be helpful to rename it to ExDocParser or something like this.

Well, in theory ex_doc can use other parsers too, and I would not like to impose that name. It also is a little bit premature I guess.

ex_doc is the raison d'être for EarmarkParser but I also will accept PRS from other clients and might implement features not used in ex_doc iff I feel they do not put too much burden on ex_docs size.

But your point is valid too

RobertDober commented 2 months ago

if EarmarkParser AST is meant to be compatible with Earmark, then it would be crucial to have Earmark.as_html(ast) function.

Simple answer: It is not

martosaur commented 2 months ago

Oh I see I got it backwards 🤦

RobertDober commented 2 months ago

Oh I see I got it backwards 🤦

trying to fix this with better doc #500