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

consolidated Enum.map |> Enum.map; conditional logic -> pattern matching #145

Merged
merged 4 commits into from
Feb 18, 2024

Conversation

bradhanks
Copy link
Contributor

No description provided.

@RobertDober
Copy link
Owner

Ty a lot for this PR, much nicer code, I will eventually remove the optimisation you have introduced (see my comment) if you can do it in the PR that would be great if not, I'll do it later

@bradhanks
Copy link
Contributor Author

I'm sure it's in a thread somewhere but it seems like earmark is going to be eventually replaced completely with earmark_parser. Is that right?

One of the reasons I ask is because I'm wondering if changes I've made the earmark will be migrated over or should I submit a new PR for the same changes in this repo?

@RobertDober
Copy link
Owner

Thanks a lot for this nice PR and followup

I'm sure it's in a thread somewhere but it seems like earmark is going to be eventually replaced completely with earmark_parser. Is that right?

No the contrary is the case, Earmark has been freed from EarmarkParser. It is now not my decision if and how Earmark will use EarmarkParser but EarmarkParser is not going to consider Earmark a client. The reason for this: EarmarkParser is for ex_doc which transforms the AST to many formats.

@RobertDober RobertDober merged commit bc84e85 into RobertDober:master Feb 18, 2024
5 checks passed
@RobertDober
Copy link
Owner

One of the reasons I ask is because I'm wondering if changes I've made the earmark will be migrated over or should I submit a new PR for the same changes in this repo?

So no, nothing will be migrated automatically. If you think a change might benefit the parser please just ping me, no need to make a PR, I will then integrate it into EarmarkParser if it fits my goals or visions for EarmarkParser.

I am not sure I had discussed this but I believe that Earmark can benefit from features (like MathML) or plugins that are useless for ex_doc.

@bradhanks
Copy link
Contributor Author

Ok thanks for the context.

You might take a look at the current version of line_scanner.ex and see what you think.

@RobertDober
Copy link
Owner

Ok thanks for the context.

You might take a look at the current version of line_scanner.ex and see what you think.

noted, thanx

@RobertDober
Copy link
Owner

RobertDober commented Feb 24, 2024

Ok thanks for the context.

You might take a look at the current version of line_scanner.ex and see what you think.

I did and this is sure of good value.

These rgxen needed to be precompiled (that was never an issue) but your code is setting a good example of how it should be done.

I am hesistant to put them all into the @rgx_map and if I do so I will change one thing. I almost always sort entries like you did, however in this special case I would probably prefer to have them in the order they are checked as this makes a big semantic difference.

I also try to remember what caused me more problems:

  • To find which was where (in that case ==> map)
  • To see them in the context of the code using it (in that case ==> @block_quote ~r{\A/\s?(.*) etc in front of each cond

I was tempted to come up with something like

@decision_table [
   { ~r/\A\z/, &empty_line/1,
     # ...
    ~r{\A/\s?(.*)}, &block_quote_line/1
    # ...      
}

def block_quote_line([line, text]) do
    %Line.BlockQuote{content: quote, line:line...
end

I am sure you see the complications with this approach too.

However

I am trying to get rid of the scanner...

and rewrite the whole parser with Minipeg

@RobertDober
Copy link
Owner

To see them in the context of the code using it (in that case ==> @block_quote ~r{\A/\s?(.*) etc in front of each cond

Oops that should not compile 😊

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.

2 participants