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

WIP Implement REPL mode for Rewrite #42

Closed
wants to merge 13 commits into from
Closed

WIP Implement REPL mode for Rewrite #42

wants to merge 13 commits into from

Conversation

MasonProtter
Copy link
Contributor

@MasonProtter MasonProtter commented Aug 22, 2018

So currently this lets one press = at the repl to open a @term> prompt. This prompt will simply take code one supplies and wrap it with @term and normalize. Hence, @term> 1 + 1 is equivalent to julia> normalize(@term 1 + 1).

I am currently working on the next step which is to allow one to declare what set of rules one wants to do with a syntax like

@term> rulest TRIGONOMETRY

which makes

@term> _______

parse to

julia> normalise(@term(_______), :TRIGONOMETRY)

This next step can either be tacked onto this PR when it's done, or it can be a separate PR depending on how eager you are to merge this one.

Ref: #27

MasonProtter and others added 3 commits August 22, 2018 11:06
Uses `convert(Term, ex)` rather than `@term` macro as a more direct approach.
@HarrisonGrodin
Copy link
Owner

HarrisonGrodin commented Aug 22, 2018

Whoa, fantastic! As far as the ruleset command goes, I'm fine either way; might be nice to include in this PR if it's not too hard, though, to give users the "full effect" upon merge.

Few quick questions:

  • Noticing that tests currently fail... any idea what's up with that? Seems like it might be from within ReplMaker itself.
  • Would it be possible to get interpolation to work (or does it already)? e.g.:
julia> x = 3;

@term> x + y
x + y

@term> $x + y
3 + y
  • Is it worth it (or possible) to test this functionality, or is it just working iff ReplMaker loads?

@MasonProtter
Copy link
Contributor Author

I’m not at my computer right now but yes, interpolation works fine iff ReplMaker loads.

The reason tests are failing is that I made the REPL mode load when Rewrite starts. To fix that, we have two options.

  1. check if Base.active_repl is defined before initializing ReplMaker

  2. make it so that the user has to evaluate a function or load a module to init the Repl mode.

I’m currently a little stuck with the rule set, but I feels like something I can get pretty quickly, so we’ll see.

@MasonProtter
Copy link
Contributor Author

Testing Repl functionality is hard because the test script does not actually spawn a Repl. A new, interactive process needs to be separately invoked (see the Replmaker tests)

We can still test it though if you’d like. I know how to do it now (kinda)

@MasonProtter
Copy link
Contributor Author

MasonProtter commented Aug 23, 2018

Do you have any preferences for the prompt text / color? Originally, I had the prompt text be Rewrite> but I figured @term> was a better description of what the REPL mode was actually doing.

Currently, the prompt is blue but that's been causing me some confusion with pkg>.

@coveralls
Copy link

coveralls commented Aug 23, 2018

Pull Request Test Coverage Report for Build 310

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.008%) to 98.964%

Totals Coverage Status
Change from base Build 270: 0.008%
Covered Lines: 382
Relevant Lines: 386

💛 - Coveralls

@codecov-io
Copy link

codecov-io commented Aug 23, 2018

Codecov Report

Merging #42 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
+ Coverage   98.95%   98.96%   +<.01%     
==========================================
  Files           8        9       +1     
  Lines         383      386       +3     
==========================================
+ Hits          379      382       +3     
  Misses          4        4
Impacted Files Coverage Δ
src/Rewrite.jl 100% <ø> (ø)
src/rule.jl 98.5% <ø> (ø) ⬆️
src/completion/Completion.jl 100% <0%> (ø) ⬆️
src/context.jl 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 527543f...4033e20. Read the comment docs.

@HarrisonGrodin
Copy link
Owner

HarrisonGrodin commented Aug 24, 2018

I like @term> - short and to the point. No major preference on color. Maybe if blue is too similar to Pkg, though, we could do :cyan or something?

Also, seems like current state is broken (ruleset undefined). Guessing that's due to not-yet-pushed testing? 😄

@MasonProtter
Copy link
Contributor Author

MasonProtter commented Aug 24, 2018

Broken? It says on my end that all 5 checks were successful...


Edit: Whoops, I misunderstood, yeah there was a spurious change still in there from my in-development version with rulesets. Should be good now.

@MasonProtter MasonProtter changed the title Implement Barebones REPL mode WIP Implement REPL mode for Rewrite Aug 24, 2018
@MasonProtter
Copy link
Contributor Author

MasonProtter commented Aug 24, 2018

Okay, so now the following syntax is supported:

@term> 1 + x with @term RULES [1 + x => y]
@term(y)

and

@term> sin(x)*inv(cos(x)) with :BASIC
@term(inv(cos(x)) * sin(x))

@term> sin(x)*inv(cos(x)) with :BASIC, :TRIGONOMETRY
@term(tan(x))

ie. the parser now will separate input based on the with keyword. It assumes for now that what follows after the with should be passed the second argument of normalize. In order to make the last example work, I needed to add a method to normalize where it will splat a tuple in the second argument. There's probably a better way of doing it though.

Next step is to add tests.

@MasonProtter
Copy link
Contributor Author

So I've added some simple tests to just make sure that

@term> ______

is equivalent to

julia> normalize(@term ______)

@HarrisonGrodin
Copy link
Owner

HarrisonGrodin commented Sep 5, 2018

Fantastic! The one thing I'm noticing is that it outputs REPL information into the test output (e.g. precompiling, REPL mode initialized, normalized result) - is it possible to slurp that up?


Edit: If you wouldn't mind, it would also be great to have the tests in their own @testset, too, for readability purposes. 🙂

@MasonProtter
Copy link
Contributor Author

Yep, no problem it was just a println statement I forgot to remove.

@MasonProtter
Copy link
Contributor Author

Okay that should do it. I see the MacOS builds stalled on 1.0 and 0.7 but not nightly. I had this problem with ReplMaker and couldn't figure out why. It's strange because the tests ran fine on my MacOS Julia 1.0 machine.

@MasonProtter
Copy link
Contributor Author

Another problem is that these tests seriously increase the test time because each time I run a test I need to spawn a new julia instance and go through the compilation process for normalize(@term _____) which is quite slow.

@HarrisonGrodin
Copy link
Owner

HarrisonGrodin commented Sep 5, 2018

Right. I wouldn't consider that a problem with the REPL mode or its tests, though - it's just another reason to fix #38.

@HarrisonGrodin
Copy link
Owner

The MacOS build failed on nightly due to REPL output comparison. We should fix that regardless, but any chance that's related to the stalling?

@MasonProtter
Copy link
Contributor Author

Since the tests are still not working on MacOS should we maybe only test the repl functionality in Linux for now?

@MasonProtter
Copy link
Contributor Author

If I remember correctly, sometimes the MacOS builds would fail for me because the precompilation would screw up the Repl output for travis MacOS.

@HarrisonGrodin
Copy link
Owner

Hmm... maybe there's no issue, but the fact that there's no output for so long while rule compilation occurs just terminates builds? (nightly just failed on Linux, which doesn't make any sense since it succeeded last time.) Maybe add a println() between tests...?

@MasonProtter
Copy link
Contributor Author

Did Nightly fail on Linux? For me, it shows success...

@HarrisonGrodin
Copy link
Owner

HarrisonGrodin commented Sep 5, 2018

Oh, I restarted it after failure, and then it passed. Arbitrary compilation timeout seems even more likely now.

@MasonProtter
Copy link
Contributor Author

The problem with this stuff is that IO is inherently non-deterministic.

@HarrisonGrodin
Copy link
Owner

Right. Let's see if this helps anything...

@MasonProtter
Copy link
Contributor Author

MasonProtter commented Sep 5, 2018

Yeah, again, precompile statements seem to be interfering. I wonder if there's a way to start Julia and tell it not to warn one that is precompiling a stale cache. I could also just disable precompilation for ReplMaker

@MasonProtter
Copy link
Contributor Author

I'm refactoring this into a separate package which re-exports Rewrite.jl. It's called RewriteRepl.

Still messing around with getting the tests to pass but I think I'm close! I think this PR can be closed.

@HarrisonGrodin
Copy link
Owner

Got it; sounds great. Thanks again! :)

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