Skip to content

Make work in 0.6 #27

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

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Make work in 0.6 #27

wants to merge 22 commits into from

Conversation

oxinabox
Copy link
Collaborator

Ok this is building on #26 and #21

I'm not saying this is the prettiest code, but it passes all tests.

In a few places I got rid of inner constructors, and replaced them with outer constructors.
They are much more sensibly behaved, being just functions.
I'ld like to do that everywhere, but for now I am happy just to have it working.

I also drop all support for 0.5.
And remove Compat.
Its just easier to stop maintaining old versions (at least til 1.0) (In my opinion)
Particularly given this package is stable so the version to version difference is likely just deprecation fixes

@oxinabox
Copy link
Collaborator Author

@coveralls
Copy link

coveralls commented Nov 26, 2017

Coverage Status

Coverage decreased (-3.03%) to 84.84% when pulling 3ea799e on oxinabox:oxff6 into a0c0da6 on andrewcooke:master.

README.md Outdated
@@ -36,7 +36,7 @@ using ParserCombinator

# the AST nodes we will construct, with evaluation via calc()

abstract Node
abstract type Node node
Copy link
Collaborator

Choose a reason for hiding this comment

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

??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh that looks like sed got away from me.

@@ -1,10 +1,10 @@

using AutoHashEquals

abstract Graph
@compat abstract type Graph end
Copy link
Collaborator

Choose a reason for hiding this comment

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

compat here

@@ -18,7 +18,7 @@ parse_dbg("ab", Equal("a") + Trace(Dot()[0:end]) + Equal("b"))

grammar = p"\d+" + Eos()
debug, task = make(Debug, "123abc", grammar; delegate=NoCache)
@test_throws ParserException once(task)
#TODO @test_throws ParserException once(task)
Copy link
Collaborator

Choose a reason for hiding this comment

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

could this be reactivated?

test/core/fix.jl Outdated
@@ -5,7 +5,7 @@ import Base: ==
signed_prod(lst) = length(lst) == 1 ? lst[1] : Base.prod(lst)
signed_sum(lst) = length(lst) == 1 ? lst[1] : Base.sum(lst)

abstract Node
@compat abstract type Node end
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove compat

@@ -20,7 +20,7 @@
@test diagnostic(LineSource("abc"), LineIter(2, 1), "bad") == "bad at (2,1)\n[Not available]\n^\n"

@test diagnostic(LineSource("l1\nl2"), LineIter(1, 2), "bad") == "bad at (1,2)\nl1\n ^\n"
@test diagnostic(LineSource("l1\nl2"), LineIter(1, 3), "bad") == "bad at (1,3)\nl1\n ^\n"
#TODO @test diagnostic(LineSource("l1\nl2"), LineIter(1, 3), "bad") == "bad at (1,3)\nl1\n ^\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

@@ -16,7 +16,7 @@ stack(0, 10)
@time println(stack(0, 100_000))
# stack limit is somewhere around 100,000 (certainly less than 200,000)

abstract Msg
@compat abstract type Msg end
Copy link
Collaborator

Choose a reason for hiding this comment

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

compat

@@ -68,7 +68,7 @@ for i in 1:10
m = match(r, s)
println("$lo $hi $s $r")
if m == nothing
@test_throws ParserException parse_one(s, Repeat(Equal("a"), lo, hi; greedy=greedy))
#TODO @test_throws ParserException parse_one(s, Repeat(Equal("a"), lo, hi; greedy=greedy))
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

@@ -52,7 +52,7 @@ m1.matcher = Nullable{ParserCombinator.Matcher}(Seq(Dot(), Opt(m1)))
@test collect(parse_all("abc", Repeat(Fail(); flatten=false, greedy=false))) == Any[[]]
@test parse_one("12c", Lookahead(p"\d") + PInt()) == [12]
@test parse_one("12c", Lookahead(p"\d") + PInt() + Dot()) == [12, 'c']
@test_throws ParserException parse_one("12c", Not(Lookahead(p"\d")) + PInt() + Dot())
#TODO @test_throws ParserException parse_one("12c", Not(Lookahead(p"\d")) + PInt() + Dot())
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

@@ -42,7 +42,7 @@
@test length(collect(parse_all("abc", p"."[1:2]))) == 2
@test parse_one("abc", p"."[3] > tuple) == [("a", "b", "c")]
@test parse_one("abc", p"."[3] > vcat) == Any[Any["a", "b", "c"]]
@test_throws ParserException parse_one("abc", And(Equal("a"), Lookahead(Equal("c")), Equal("b")))
#TODO @test_throws ParserException parse_one("abc", And(Equal("a"), Lookahead(Equal("c")), Equal("b")))
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

@test parse_one("abc", Seq(p"."[1:2], p"."[1:2])) == ["a", "b", "c"]
@test parse_one("abc", Seq(p"."[1:2,:&], p"."[1:2])) == Any[["a"], ["b"], "c"]
@test parse_one("abc", Seq(p"."[1:2,:&,:?], p"."[1:2])) == Any[["a"], "b", "c"]
@test_throws ErrorException parse_one("abc", Seq(p"."[1:2,:&,:?,:x], p"."[1:2]))
#TODO @test_throws ErrorException parse_one("abc", Seq(p"."[1:2,:&,:?,:x], p"."[1:2]))
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

@CarloLucibello
Copy link
Collaborator

Hi, thanks for doing this, I got stucked at a certain point.

There are a few tests that cuold/shoud be reactivated (where you see TODO), and some @compat calls in test/ that could be removed

@oxinabox
Copy link
Collaborator Author

Thanks for the feedback.
I'll poke this tomorrow or so.
Huh, I didn't realize that the tests had compat nor that they were changed .(I guess they were changed in your PR)

@coveralls
Copy link

coveralls commented Nov 27, 2017

Coverage Status

Coverage decreased (-2.8%) to 85.118% when pulling 2f7267f on oxinabox:oxff6 into a0c0da6 on andrewcooke:master.

@oxinabox
Copy link
Collaborator Author

oxinabox commented Dec 6, 2017

Bump.
Looks like that worked, still losing a few lines on coverage though.

@CarloLucibello
Copy link
Collaborator

@andrewcooke could you grant write permissions to @oxinabox and me?

@oxinabox
Copy link
Collaborator Author

oxinabox commented Jan 4, 2018

@CarloLucibello can you review and merge if ready?

@CarloLucibello
Copy link
Collaborator

CarloLucibello commented Jan 5, 2018

tests are passing, although the output around this line looks not properly formatted. I don't know if it is just a problem with tests or we are really loosing something. If you manage to fix this good, otherwise let's just merge.

@oxinabox
Copy link
Collaborator Author

oxinabox commented Jan 5, 2018

I can't see any changes that could have been causing that on a quick look.

But the last builds for the 0.5 current master don't screw up like that
https://travis-ci.org/andrewcooke/ParserCombinator.jl/jobs/152686452

@oxinabox
Copy link
Collaborator Author

Oh did this never get merged?

@CarloLucibello
Copy link
Collaborator

tests are passing and there are no warnings left. I think there are still some problems to be addressed though, some part of the tests' output looks incorrect:

/4        expr 3.0-4.0-4.0*(3.0*7.0+0.0-0.0)+6.0*3.0+2.0 
09-65.0 -65.0 
         calc okDepth
->one levelAlt

    66::ab        /4          0101  AltTrace->->divDot

    22::/4        b           110 0  div->TraceSeq<-
['a'] 
 multiple7 
:  4         2  :212ab        :  b           0 Seq00-> 0neg 
Trace ->Trace Dot->7
Depth:   :
4                       1300 0:    b         Alt ->Trace0Transform<-!!!
1
    Depth7->:Dot4         
  14  1    :Transform          -> Pattern0
1   :             Depth14<- ['b']    
Transform <-!!!
    :3          :           13  0   1Alt <-!!!
  Depth ->7Dot:

@@ -33,7 +26,7 @@ function mk_parser(string_input)
comment = P"(#.*)?"

# we need string input as we match multiple lines
if string_input && ParserCombinator.FAST_REGEX
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@CarloLucibello Do you know why ParserCombinator.FAST_REGEX was removed from this line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah becaue it is always true now

@oxinabox
Copy link
Collaborator Author

I can't workout (just from reading) what is causing all these extra displays.
I am tempted to try and mock STDOUT with something that will throw an exception at the point where the extra output is printed.

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.

3 participants