LuaCATS / lpeg

Definitions for the LPeg library
MIT License
2 stars 4 forks source link

Name collision #11

Open jlaurens opened 4 months ago

jlaurens commented 4 months ago

Pattern is a general name also used by some frameworks. lpeglib.Pattern seems better than just lpeg.Pattern.

Josef-Friedrich commented 3 months ago

I would like to keep the lpeg prefix. What name would you want to use instead of Pattern?

jlaurens commented 3 months ago

I have the impression that keeping the lpeg prefix might not be a good idea because the lpeg namespace is not de facto free. After some tests, it happens that for my own usage, the distinction between Pattern and Capture does not help that much. So I have patched things for my own usage such that you don't need to make any change. I use lpeglib.Pattern to mimic builtin oslib, iolib`...

Josef-Friedrich commented 3 months ago

In this repo the lpeg table is local

https://github.com/LuaCATS/lpeg/blob/36adfea9e37c6ec41e20a33634e11b5062ec4863/library/lpeg.lua#L42

The name of the table should therefore have no effect on code outside.

In the LuaTeX repo lpeg is global

https://github.com/LuaCATS/tex-luatex/blob/4c2c8fd3bbc31cf8c0af2b6389ca598d2a935f52/library/lpeg.lua#L48

So I just had to delete the local keyword and was able to synchronize the two type definitions: The type defintions for the upstream LPeg project and the type defintions for the embedded LPeg (and outdated version) in LuaTeX

Josef-Friedrich commented 3 months ago

If I remember correctly, the distinction between Pattern and Capture was introduced by @AndreasMatthias. May be he can help out?

AndreasMatthias commented 3 months ago

AFAIK types Pattern and Capture existed since the beginning. They were not introduced by me. But what I changed then, was making Capture an alias for Pattern such that LuaLS knows how these two types relate to each other. Without this alias some operations had an undefined result type if I remember correctly.

Whether or not aliases are expanded by LuaLS depends on the configuration setting hover.expandAlias. See https://github.com/LuaCATS/lpeg/blob/main/README.md and https://luals.github.io/wiki/settings/#hoverexpandalias.

I usually disable hover.expandAlias since I prefer to be reminded whether I'm dealing with a Capture or a Pattern.

jlaurens commented 3 months ago

I find the distinction between Capture and Pattern quite blurry

In general, if Capture is similar to a "pattern with captures" then probably some additional Captures should be recognized.

The only interest I see in Capture is that lpeg.B should no accept Capture. In that case, the Capture recognition seems very loose. Something else?

Josef-Friedrich commented 3 months ago

@AndreasMatthias Thank you for the clarification!

Josef-Friedrich commented 3 months ago

@jlaurens If I understand you correctly, you would merge Pattern and Capture under a new more specific name (PatternWithCapture)? How would you handle the lpeg.B corner case? That would be a major API breaking change? Than we should put up a warning in the README or in the type defintions?

Feel free to submit a PR.