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

Java REPL #9

Merged
merged 19 commits into from Feb 23, 2017

Conversation

@jgkamat
Copy link
Member

commented Feb 19, 2017

This is still a WIP, but its mostly done.

Things that need to be addressed

  • Right now, this parses the history to get instruments: (for the prompt, eg: p>), which shold probably be part of a request to the alda server. I'm not sure how to do this though...
  • :play [from] [to] seems to be broken right now (and needs to be smoothed out, and possibly debugged)
  • Alda server history issues need to be resolved (part of new parser) before this can be merged, or this will break the tutorial.
  • We need to decide if it's worth it to drop java7. I didn't use java8 features as much as I'd thought I would, so I can convert it back (but this would become even more verbose...)
  • I did a few hacky things in AldaServer.java, which probably needs to be revisited
  • It might be better to store all the commands in seperate files (in it's own package) instead of making them all as private inner classes. (It would be cleaner, but harder to read).
  • I tried to keep everything as close to the existing repl as possible, but it might be better to change some things while we're messing around with this (to improve them).
  • I dropped the :map command, because I'm not sure how to query the server to get the map right now. It would probably be hard to read in java though (maybe we could use the cojure interop for this, but it might be slow?) I'm not sure if we can drop :map, or if we should add the command to get that from the server.

(side note: having an :edit command with java might be a pain, as we would need to manually connect up stdin, stdout, and stderr and monitor all of them. If we didn't do this, gui editors would still work, but cli editors would break (like vi) which would be pretty bad. see: http://steveliles.github.io/invoking_processes_from_java.html).

Other than that, this has support for the following commands:

:play
:new
:load
:save
:quit
:help

Let me know what you think, and if there's any other (small for now) commands I could add. There's probably a lot of small mistakes in here too, which I'll fix as they come up :P.

Closes #1

jgkamat added 9 commits Jan 28, 2017
Still need error checking, keywords, and dynamic prompts
Examples are :help and :score
Right now nothing works
Implemented:
:play
:help
:new
:quit
:score
Fix some problems in :play
Far from the best solution, but it's something. Eventually we should
probably poll the server to get the current-instrument from it's map.

This would also let us make the :map command again.
@jgkamat jgkamat added this to the 1.0.0 milestone Feb 19, 2017
@jgkamat jgkamat self-assigned this Feb 19, 2017
@jgkamat jgkamat requested a review from daveyarwood Feb 19, 2017
Copy link
Member

left a comment

This looks fantastic! Really great work! I've left some code feedback here, and will leave a follow-up comment below re: where to go from here.

build.boot Outdated
(when-not (empty? jdk8-bootclasspath)
["-source" "1.8"
"-target" "1.8"
"-bootclasspath" jdk8-bootclasspath])))}

This comment has been minimized.

Copy link
@daveyarwood

daveyarwood Feb 20, 2017

Member

I think we'll be able to just remove this and use ["-source" "1.8" "-target" "1.8"] as the javac options. This worked for me, I assume because I have JDK 1.8 installed as the default. We could always do a similar thing in the future when Java 9 becomes more ubiquitous.

This comment has been minimized.

Copy link
@jgkamat

jgkamat Feb 20, 2017

Author Member

I don't know boot too well, so I was kind of shooting in the dark here :P. I'll switch this over in a bit.

Definetly let me know if I messed anything up in here. Also, it's fine to upgrade this boot file and leave the other projects as they are for now, right?

if (catchExceptions)
error("No worker address included in response; unable to check for status.");
else
throw new ServerRuntimeError("No worker address included in response; unable to check for status.");

This comment has been minimized.

Copy link
@daveyarwood

daveyarwood Feb 20, 2017

Member

There is some repeated code here -- I wonder if it would make sense for the error function to check catchExceptions and either print the error message or throw a ServerRuntimeError?

if (catchExceptions)
error(res.body);
else
throw new ServerRuntimeError(res.body);

This comment has been minimized.

Copy link
@daveyarwood

daveyarwood Feb 20, 2017

Member

see comment below

if (catchExceptions)
error(update.body);
else
throw new ServerRuntimeError(update.body);

This comment has been minimized.

Copy link
@daveyarwood

daveyarwood Feb 20, 2017

Member

see comment above

out = ansi().fg(color).a(out).reset().toString();
}
return out;
}

This comment has been minimized.

Copy link
@daveyarwood

daveyarwood Feb 20, 2017

Member

nice! 👍


// If we have no exceptions, add to history
history.append(input);
history.append("\n");

This comment has been minimized.

Copy link
@daveyarwood

daveyarwood Feb 20, 2017

Member

exactly what I had in mind 👍

@Override
public void act(String args, StringBuffer history, AldaServer server, ConsoleReader reader) {
// Turn ~ into home
args = args.replaceFirst("^~",System.getProperty("user.home"));

This comment has been minimized.

Copy link
@daveyarwood

daveyarwood Feb 20, 2017

Member

For some reason, this did not work on my Macbook:

> :load ~/Code/alda-core/examples/hello_world.alda
There was an error reading '~/Code/alda-core/examples/hello_world.alda'

# (the command below worked)
> :load /Users/dave/Code/alda-core/examples/hello_world.alda

It's not really a show-stopper, but figured I'd mention it anyway.

This comment has been minimized.

Copy link
@jgkamat

jgkamat Feb 20, 2017

Author Member

Huh, thats definitely pretty weird...

Can you try compiling the following code snippet and see what it prints out for you? (for me, it prints out /home/jay)

public class Main {
	public static void main(String[] args) {
		System.out.println(System.getProperty("user.home"));
	}
}

I'd rather try to get this working everywhere before this is merged :P

});
history.delete(0, history.length());
history.append(newHistory);
// TODO check to see if score is valid by sending it to server (maybe with :to 0:00)

This comment has been minimized.

Copy link
@daveyarwood

daveyarwood Feb 20, 2017

Member

This is a good idea. We could use alda parse --map for to parse and evaluate the score without playing it.

@daveyarwood

This comment has been minimized.

Copy link
Member

commented Feb 20, 2017

One more minor note:

  • I noticed that if I quit the REPL by pressing Ctrl-D, I get a weird null error from the server:

    > [27714] ERROR null
    

    This does not happen when I press Ctrl-C, or if I type :quit.


  • Right now, this parses the history to get instruments: (for the prompt, eg: p>), which shold probably be part of a request to the alda server. I'm not sure how to do this though...

I think this could be something we return from the server on every play request. As of now, we send back a "Request received" message, but instead, we could send back the map of score info that results from evaluating a score. This map contains a :current-instruments key, which is what we can use to generate the prompt.

  • We need to decide if it's worth it to drop java7. I didn't use java8 features as much as I'd thought I would, so I can convert it back (but this would become even more verbose...)

I'm on board with dropping Java 7.

  • It might be better to store all the commands in seperate files (in it's own package) instead of making them all as private inner classes. (It would be cleaner, but harder to read).

I think this is a good idea -- otherwise, the single file of REPL commands will get large and unwieldly over time.

  • I dropped the :map command, because I'm not sure how to query the server to get the map right now. It would probably be hard to read in java though (maybe we could use the cojure interop for this, but it might be slow?) I'm not sure if we can drop :map, or if we should add the command to get that from the server.

The command to get that from the server is alda parse --map:

$ alda parse --map -f ~/Code/alda-core/examples/hello_world.alda
{"chord-mode":false,"current-instruments":["piano-lj5dg"],"events":[{"offset":250.0,"instrument":"piano-lj5dg","volume":1.0,"track-volume":0.7874015748031497,"panning":0.5,"midi-note":62,"pitch":293.6647679174076,"duration":225.0,"voice":null},{"offset":1750.0,"instrument":"piano-lj5dg","volume":1.0,"track-volume":0.7874015748031497,"panning":0.5,"midi-note":62,"pitch":293.6647679174076,"duration":225.0,"voice":null},{"offset":2000.0,"instrument":"piano-lj5dg","volume":1.0,"track-volume":0.7874015748031497,"panning":0.5,"midi-note":60,"pitch":261.6255653005986,"duration":1350.0,"voice":null},{"offset":1250.0,"instrument":"piano-lj5dg","volume":1.0,"track-volume":0.7874015748031497,"panning":0.5,"midi-note":65,"pitch":349.2282314330039,"duration":225.0,"voice":null},{"offset":500.0,"instrument":"piano-lj5dg","volume":1.0,"track-volume":0.7874015748031497,"panning":0.5,"midi-note":64,"pitch":329.6275569128699,"duration":225.0,"voice":null},{"offset":1500.0,"instrument":"piano-lj5dg","volume":1.0,"track-volume":0.7874015748031497,"panning":0.5,"midi-note":64,"pitch":329.6275569128699,"duration":225.0,"voice":null},{"offset":750.0,"instrument":"piano-lj5dg","volume":1.0,"track-volume":0.7874015748031497,"panning":0.5,"midi-note":65,"pitch":349.2282314330039,"duration":225.0,"voice":null},{"offset":1000.0,"instrument":"piano-lj5dg","volume":1.0,"track-volume":0.7874015748031497,"panning":0.5,"midi-note":67,"pitch":391.99543598174927,"duration":225.0,"voice":null},{"offset":0,"instrument":"piano-lj5dg","volume":1.0,"track-volume":0.7874015748031497,"panning":0.5,"midi-note":60,"pitch":261.6255653005986,"duration":225.0,"voice":null}],"beats-tally":null,"instruments":{"piano-lj5dg":{"octave":4,"current-offset":{"offset":3500.0},"key-signature":{},"config":{"type":"midi","patch":1},"duration":{"beats":3.0,"ms":0},"min-duration":null,"volume":1.0,"last-offset":{"offset":2000.0},"id":"piano-lj5dg","quantization":0.9,"duration-inside-cram":null,"tempo":120,"panning":0.5,"current-marker":"start","time-scaling":1,"stock":"midi-acoustic-grand-piano","track-volume":0.7874015748031497}},"markers":{"start":0},"cram-level":0,"global-attributes":{},"nicknames":{},"beats-tally-default":null}

Overall, I think this PR is 100% awesome, and it gets us most of the way to a faithful reproduction of the existing Clojure Alda REPL.

If you don't mind making some minor code updates based on my feedback, I'd be happy to merge this PR as-is, and we can spin off new issues for each of the WIP items like getting the instruments for the prompt back from the server, etc.

Thanks again, this is great! 🎸 🎶

jgkamat added 2 commits Feb 20, 2017
@jgkamat

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2017

Ah, yah I completely forgot about the null problem, I'll make sure thats fixed in the next commit. I'd rather try to get the history validation working (and ideally the custom prompt, I'm not sure how much work server side that would be) before this is merged. Maybe I could use the results from alda parse to get the current instrument (but I would need to send more requests to the server...)

I'll work on moving all the commands out (and fixing these issues), and I'll let you know when this is ready!

Thanks! 😄

jgkamat added 5 commits Feb 20, 2017
They are now independent files in alda.repl.commands
Hopefully this dosen't break building the release.
TODO: Needs to be formatted pretty, instead of just printing raw results
@jgkamat

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2017

I think I've addressed all the points above (except the custom prompt), let me know if I missed anything.

The last large issue that I can have is trying to find a way to parse the output of alda play --map in the repl. I was thinking I could use the clojure interop to parse it into a clojure map (and pretty print/parse it from there), but I don't think maps can be read directly into clojure (like python). Is there a way to do this? (I don't know the clojure stdlib too well...)

The alternatives to this would be to do both:

  1. return a pretty formatted map when requested (thereby removing the need for it in the :map command
  2. return the current instrument from a play requset, so we don't need to query the server to see which instrument.

Also, another thing to remember, is that this probably will break the tutorial until the new parser is implemented in the server, so don't make a new release (or just hold of on merging this) until that's done!

build.boot Outdated
;; "JDK7 classpath jar, e.g. (OS X example) "
;; "/Library/Java/JavaVirtualMachines/jdk1.7.0_71.jdk/Contents/"
;; "Home/jre/lib/rt.jar"))
;; fileset))

This comment has been minimized.

Copy link
@daveyarwood

daveyarwood Feb 22, 2017

Member

It would be fine to just remove these lines entirely.

These are the old lines needed for jdk7 support
@daveyarwood

This comment has been minimized.

Copy link
Member

commented Feb 23, 2017

The most common way to deal with JSON is Clojure is the excellent Cheshire library. It's most commonly used to parse a JSON string into a Clojure map, or to generate a JSON string from a Clojure data structure. Here's a quick demo:

boot.user=> (require '[cheshire.core :as json])
nil
boot.user=> (json/generate-string {:a 1 :b "two" :c :three :d [4 {5 6}]})
"{\"a\":1,\"b\":\"two\",\"c\":\"three\",\"d\":[4,{\"5\":6}]}"
boot.user=> (json/parse-string "{\"foo\": [\"bar\", {\"baz\": 42}]}")
{"foo" ["bar" {"baz" 42}]}
; pass an extra "true" argument to convert string keys to Clojure keywords
boot.user=> (json/parse-string "{\"foo\": [\"bar\", {\"baz\": 42}]}" true)
{:foo ["bar" {:baz 42}]}

This has me thinking... since alda parse --map returns JSON, maybe pretty-printing the JSON should be the behavior of :map in the REPL. I figure that JSON is much more widely known and supported, so it's a good default data format for the Alda score map.

return the current instrument from a play requset, so we don't need to query the server to see which instrument.

I think this is a good idea -- otherwise, it might be possible to enter a weird state where, for example, the play request succeeds, but the parse request fails (e.g. if the server goes down between the requests).

@daveyarwood

This comment has been minimized.

Copy link
Member

commented Feb 23, 2017

Also, another thing to remember, is that this probably will break the tutorial until the new parser is implemented in the server, so don't make a new release (or just hold of on merging this) until that's done!

Roger that -- I'm planning on pushing a new release of Alda after we have things in a decent working order with the new Java REPL and the --history feature.

@jgkamat

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2017

Oh, I completely blanked that --map returned standard json (I thought it was clojure for some reason, and I haven't worked with json too much). Do you reccomend I use gson (I think I'm using it for the update script) and find a way to pretty print that, or should I use the clojure bridge with cheshire library (and pretty print with the pretty print function)? I think I could pretty easily add both of those methods.

While I'm at it, should alda parse --map print the json in pretty form too?

I'll hold off on the instruments until a later pull request (so the server side part can be thought out properly :P). The heuristic for guessing names works ok-ish, but it definetly should be fixed later on.

@daveyarwood

This comment has been minimized.

Copy link
Member

commented Feb 23, 2017

I haven't pretty-printed JSON in Java before, but it looks like Gson can do it fairly easily.

I think the CLI version (alda parse --map) should NOT pretty-print the JSON. It's more flexible that way, and the output can be piped to something like jq if pretty-printed output is desired.

@daveyarwood

This comment has been minimized.

Copy link
Member

commented Feb 23, 2017

I think this might be good to merge now -- so, TODO at this point:

  • alda-server-clj: adjust the response for alda play so that it includes the JSON data
  • alda-client-java: when playing each line of code at the REPL, parse the response JSON to get the current instruments and update the prompt
  • alda-core: (in progress by me) rewrite parser and ensure that score evaluation fails in situations like trying to play notes before there are instruments defined

Does that sound about right, @jgkamat?

@jgkamat

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2017

Yup, that sounds like the state of this. I'm fine with merging this right now, but if you want to wait for the real instrument data to be returned by play, I'm fine with that too. Your call!

I'll be a little busy next week (so I probably won't be able to work on anything big) but I'll be able to fix small bugs as they come up.

@daveyarwood

This comment has been minimized.

Copy link
Member

commented Feb 23, 2017

Sounds great -- thanks again for all your work on this, it's turning out great already!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.