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

Some suggestion for a new major version #150

Open
fy0 opened this issue May 8, 2024 · 2 comments
Open

Some suggestion for a new major version #150

fy0 opened this issue May 8, 2024 · 2 comments

Comments

@fy0
Copy link

fy0 commented May 8, 2024

These are just some thoughts I have when using it, they might not be correct, and only for reference. My English is not very good, so please excuse any offensive language because I might not realized. I will implement some features in my fork for preview, but I'm not proactive in writing tests.

  • remove state from pigeon. In my project's test, piegon is 15x slower than pointlander/peg(4.5s vs 0.3s), 7x because of i write and check a value in state(4.5 -> 2.3), and 3x because of empty state clone(2.3 -> 1.4), it's a terrible feature. Never enable it.

  • Consider about not return val by parseExpr, or replace by somthing. Just removed vals := make([]any, 0, len(seq.exprs)) from parseSeqExpr make my interpreter tests's time cost reduced from 1.4s to 0.85s. (My interpreter generate bytecode during parse, so i don't care what p.parse returned).

  • [implemented] Cancel the limitation of actionExpr(the most common way of writting golang code in peg file), we can write grammer like: expr <- firstPart:[0-9]+ { ... } secondPart:[a-z]+ {}

  • [implemented] Expose parser to actionExpr. The design of pigeon is assuming result of parse is AST and use peg grammer strictly. User should not care the parser, even can't access it. It's no need to hide at all.

  • [implemented] actionExpr need to return value, error, but in actual use, we hardly ever return an error. Meanwhile, when the parser is passed in, in a few necessary cases, we can directly call p.addError. Just return value and add return c.text at funcCallTemplate, it's no need to require a return expression.

  • Use stream (io reader)

  • [implemented] Remove exported API like Parse ParseFile ParseReader and so on. These apis return any to user, I don't think it's a good idea. Don't make them exported, let me decide what shoud be exported.

  • Change the design of Option , i think it's a side effect of hidding parser. it's over-abstraction.

  • [implemented] Provide a struct to embed, to replace the global state.

  • [implemented] Support string capture, it's a syntax from pointlander/peg: capture <- x:<'capture'> { fmt.Println(x) }

@breml
Copy link
Collaborator

breml commented May 16, 2024

Hi @fy0
Thanks for you suggestions (also in your other issues). At the moment, I do not have a plan nor the resources to work on a new major release. I mainly focus on the maintenance of the existing version. This is also the reason, why I am currently slow in replying to your issues.

@fy0
Copy link
Author

fy0 commented May 16, 2024

Hi @fy0 Thanks for you suggestions (also in your other issues). At the moment, I do not have a plan nor the resources to work on a new major release. I mainly focus on the maintenance of the existing version. This is also the reason, why I am currently slow in replying to your issues.

I can understand. I like this project, because code is clear, it has a good structure and easy to customize.
But there's also problems exists for years, some of them are related to basic design and won't be fixed unless make breaking change(for example, the critical proformance problem). Therefore, it can't be a feature request, that why i named it 'new major suggestion' but it's not request for a new marjor verison.

And I agree with what you said, it's a difficult and requires a lot of resrouces to build a new marjor version.
I thought i wasn't the only one found these problems, I just recorded them, maybe it will helps.

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