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

Major refactoring and new API (among others for compat with Julia v0.5 and above) #24

Closed
wants to merge 53 commits into from

Conversation

vonDonnerstein
Copy link

Dear Abe,

since the code before this pull request did not work with Julia 0.5 and above, I spent the better part of my holidays recreating the library design. The new design does not hook into julia's own parsing logic (via macros) anymore and is - I believe - more clearly structured.

After having done quite a lot of work on this, I came to realize that the license file was actually just a copy-paste from Cairo.jl and thus didn't even apply to PEGParser.jl. I believe however that it was your intent to put the code under MIT license (as Cairo.jl is). I have updated the license file accordingly. If I am mistaken on this, please let me know and don't accept the pull request. While being at it I allowed myself to add my name as well as yours to the license file (because I feel that my work is a considerable contribution to the code base :) ). You are very welcome to integrate all of my work into the code base (i.e. accept this pull request) under the premise of keeping my name in the license file.

As the API has changed rather drastically by this refactoring I suggest one of two ways to go:

  1. accept the pull request (including updated readme, examples, tests,...) and tag the HEAD as a new major release to inform users of the library about the possibly breaking changes (in this case tagging to 0.2.0 or even 1.0.0 as you see fit)
  2. do not accept the pull request and I will register the code as a new Julia package

The second options may seem to have the advantage of not requiring any code update to packages using PEGParser. Let me just note in passing: These can only be comparably old codes which have not yet updated to j0.5 or j0.6, anyways. But feel free to decide and let me now which of the two works best for you.

Have a nice day,
Henry

@abeschneider
Copy link
Owner

Hi Henry,

I'm more than happy to accept any changes (especially ones that keep PEGParser current, as much of my time is currently being spent on other projects). However, I think one of the biggest strengths of PEGParser over other similar libraries, is the use of the macros over the strings. It has the advantages of: (1) the grammar can be checked at compile time; (2) no separate grammar parser is required; (3) macros (and multi-dispatch) are one of the core strengths of Julia.

If you wanted to commit changes that kept the macros, but still upgraded things for v0.5, that would be a clear path forward for PEGParser. If you rather keep the string-based approach, it probably makes more sense to start a new projects.

Thanks for all the work!
A

@vonDonnerstein
Copy link
Author

Hey Abraham,

Thank you for clarifying. I see your points. I guess it does make sense to have both designs/packages then. If you agree, I will publish the rewritten code as a new package under the MIT license naming both of us. Is that ok with you?

Best regards,
Henry


```julia
@grammar <name> begin
Grammar("""
rule1 = ...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to match the new examples, which have rule => ...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late reply -- no need to put my name on the library since it looks like it's taking a different direction. Instead, if you like, you can just put based on PEGParser.

@ScottPJones
Copy link
Contributor

ScottPJones commented Jan 18, 2017

I hope you can also cherry pick your commits that fix the licensing info for PEGParser.jl and make it work for v0.5 (Maarten [Heremans, my coworker] and I had previously provided a few fixes and made it work for v0.4).
Also, when/if you make your own version (after squashing that huge number of commits ;-) ), what would you name it (since it is incompatible with the original PEGParser.jl)?
Note: I think at least one of Abe's concerns can be addressed by simply adding:

macro grammar_str(str)
    Grammar(str)
end

so that the grammar checking is done at compile time.

Is there something that documents the syntax differences between the two? (such as = now being =>)?

Also, would it be possible to allow for the current macro forms, as well as this string based form (and a string macro as I suggested above) in the same package? How much of the code would still be in common, with all of this refactoring that has been done, in that case?

Finally, I'd love to have one or both versions of PEGParser.jl be moved to the JuliaString organization I'm trying to pull together, so that things like maintenance to keep up with the quick pace of Julia development (v0.6 is just around the corner) can be done by more people.

@abeschneider
Copy link
Owner

I agree with @ScottPJones, it would be really appreciated if you could do a PR just for the changes to get PEGParser compatible with v0.5 of Julia.

Also, if people are using the library, I can devote a little time this weekend to getting the library upgraded. I haven't used Julia for a while now, so unless an Issue or PR comes along, I won't know things need to be fixed.

@ScottPJones
Copy link
Contributor

I think we are still using the library (at work, not me personally), but we're still on v0.4.7, which is why we haven't made a PR for moving to v0.5 or the dev v0.6 yet.

@ScottPJones
Copy link
Contributor

ScottPJones commented Jan 18, 2017

Abe, what do you think about moving it to the JuliaString org (I don't know if another org would be more applicable), if you currently are not using Julia (I hope that changes in the future 😀), so that that sort of maintenance can be done without you having to get involved. (I think you'd still have commit access on that repo, or I could add that, if it doesn't happen automatically)

@abeschneider
Copy link
Owner

@ScottPJones That sounds like a good idea. I'll likely come back to Julia and happy to continue to support my current libraries, but right now Swift currently has most of my attention.

@vonDonnerstein
Copy link
Author

First of all, thanks to @ScottPJones for the macro idea. Also, @abeschneider thanks for allowing me to put up the rewritten code as a new library. I will of course cite PEGParser at the top of the readme and in the license.

I'm very sorry that I haven't made myself clear enough in my previous posts. I will try to do better now:

Explanation:
In any of the two designs one first needs to convert a legible grammar-declaration into a Grammar-object. Then this Grammar object and the String are given to parse() for the actual parsing step. The main difference between the two approaches is what to use for the first step.

The orginal approach uses the julia language parser itself during the first step by first having the declaration parsed as an expression (begin ... end) and then modifying this by the @grammar-macro.
This was no problem when switching from j0.3 to j0.4, so only String had to be replaced with AbstractString everywhere. But this time something in the principal parsing logic of julia seems to have changed (probably due to the introduction of "all functions having their own type"). This means that I wasn't able to find what had to be changed in a reasonable amout of time.

The new design in contrast builds a grammar from the low-level functionality of the library once. This is then used to parse declarations into Grammar-objects for all further grammars. Because this low-level functionality only depends on functions and types and not on how julia itself parses its expressions this is much more robust against changes of the language. E.g. I rewrote the design on j0.4 and when I tried it with j0.5 it just worked.

Conclusion:
It is not possible to cherry-pick the things that fix the original design for j0.5 from my code, because I didn't ever get that design to work in j0.5. Maybe @abeschneider can fix it, but for me it seemed much easier to simply take an approach which is not so deeply nested with the julia-internals.

I hope you will manage to fix the current code for j0.5 (for an example of where it fails, simply try the examples in the readme). While I understand the elegance of using the julia-parser, I believe there will always be a need for easily maintainable code, too. So I will put the new code up as a separate library. As it is a String-based parser for Strings I will probably name it "StringParser.jl". Any better suggestions? Also, I will link this discussion in the beginning of the readme, so people can better decide which code to use.

@ScottPJones
Copy link
Contributor

I think the name "StringParser.jl" might be a bit too general. Maybe "StringPEGParser.jl"?
(since PEG parsers can't handle CFG grammars, for example)

@ScottPJones
Copy link
Contributor

OFF-TOPIC: @abeschneider I'd be interested to hear about your explorations into Swift. I'm also interested in it (I based my StringUtils.jl string literal syntax off of Swift's string literals, much cleaner than C's, IMO, which Julia copied pretty much without change, except for the $ interpolation (which I dislike enormously!))

@abeschneider
Copy link
Owner

@vonDonnerstein Okay that makes sense. When I originally wrote PEGParser, there wasn't much in the way of tools for working in macros. I suspect this has changed, so one option is to switch to using a well defined library.

@ScottPJones I really like Swift. Like Julia, it has a bunch of warts due to being a growing/changing language. The major plus sides: (1) also built on the LLVM so can be both an interpreted and compiled language; (2) Unlike Java, takes many of the powerful features of C++ and simplifies them in a nice way (e.g. generics in Swift are much closer to Templates, but have constraints [whereas c++ still doesn't have Concepts]); (3) It has really nice operator-overloading semantics; (4) While it doesn't allow for multiple inheritance, a mix of Protocols and Extensions are a nice alternative.

Most of my free time has been spent writing a Tensor library in Swift (https://github.com/abeschneider/stem)

@vonDonnerstein
Copy link
Author

Hi! I'm sorry it took me so long, but I have finally published the String based variant as discussed. I've called it StringParserPEG - similar to ScottPJones' suggestion. This pull request is therefore no longer necessary and I'm closing it now. Thanks again everyone!

@Hugo-Trentesaux
Copy link

👏

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.

4 participants