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

Parser situation is confusing #493

Closed
martosaur opened this issue May 27, 2024 · 11 comments
Closed

Parser situation is confusing #493

martosaur opened this issue May 27, 2024 · 11 comments

Comments

@martosaur
Copy link
Contributor

martosaur commented May 27, 2024

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.

@martosaur martosaur changed the title Parses situation is confusing Parser situation is confusing May 27, 2024
@amit-chaudhari1
Copy link
Collaborator

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
Copy link
Collaborator

Well the basic idea was:

  • EarmarkParser was created to serve ex_doc
  • Earmark might want features that are just not interesting for ex_doc hence the code of EarmarkParser pushed into Earmark.
  • Using EarmarkParser again in Earmark will not work as I am getting rid of all options that are not relevant for ex_doc

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

@martosaur
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor Author

Oh I see I got it backwards 🤦

@RobertDober
Copy link
Collaborator

RobertDober commented Jul 8, 2024

Oh I see I got it backwards 🤦

trying to fix this with better doc #500

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

No branches or pull requests

3 participants