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

Lexer behaviour #45

Open
tmbb opened this issue Feb 27, 2022 · 5 comments
Open

Lexer behaviour #45

tmbb opened this issue Feb 27, 2022 · 5 comments

Comments

@tmbb
Copy link
Collaborator

tmbb commented Feb 27, 2022

Pinging @josevalim because of the discussion on the NimbleParsec repo.

Currently, the lexer behaviour defines these two callbacks: https://github.com/elixir-makeup/makeup/blob/master/lib/makeup/lexer.ex#L8-L17

However, when I defined this behaviour, I was thinking of reusing combinators from one lexer inside another lexer. The goal would be to be able (for example) to create an EEx lexer that would call the Elixir lexer for the Elixir part. This means that the root_element and the root combinators should be accessible from outside of the Lexer as combinators (not only as functions).

My question is:

  1. Should we keep the behaviour as it is and add combinators corresponding to these entrypoints? (that is, a root_element__0/6 function and a root__0/6 function?
  2. Should we change the behaviour so that only the combinators (i.e. fun__0/6) are exported?

I think that for backward compatibility reasons we should go with option 1. Also, the way we are implementing the lex/2 function in most lexers is based on a the fact that a root/1 function exists.

What do you think @josevalim? This would be a good case for the export_combinator: true option you've talked about?

@josevalim
Copy link
Collaborator

I am fine either way. You could also do something like this:

defcombinator :root_combinator, Mod.root(...)
defparsec parsec(:root_combinator)

export_combinator: true will literally be like 3 LOC, so i don't mind adding it at all. Let me know what you prefer and I am good.

@tmbb
Copy link
Collaborator Author

tmbb commented Feb 27, 2022

I am fine either way.

Ok, so let's go with option 1 where I add two new callbacks to the lexer behaviour. It doesn't break backward compatibility, which is good.

export_combinator: true will literally be like 3 LOC, so i don't mind adding it at all.

I'd prefer if you added that to nimble_parsec. Not because it would be too much work to add this to makeup lexers but because it makes it clearer that a combinator is not exported by default and that there is an option that would make it exportable by default.

@josevalim
Copy link
Collaborator

I will push tomorrow morning the latest and do a new release.

@josevalim
Copy link
Collaborator

NimbleParsec v1.2.3 is out!

@josevalim josevalim reopened this Feb 28, 2022
@tmbb
Copy link
Collaborator Author

tmbb commented Mar 7, 2022

I haven't updated the behaviour yet, but I think I'll make the two new callbacks optional so that old lexers don't need to update. Even in this case, I'm not sure how I'll handle the @impl true attribute (which should be added both for the combinator and for the parsec, which is impossible if I'm using export_combinator: true).

The important part is that the latest version of the Elixir lexer already implements the two new combinators, which means I could reuse it for my (H)EEx lexer (which I'll publish today).

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

2 participants