Instructions print via game #32

drewish · 2017-10-30T14:24:39Z

This is a bit of a work in progress. I started out trying to get the Instruction objects sending their output to the game so my subclass could capture it. At that point it seemed pretty natural to try to handle a bit of #21. I didn't go all the way but it removes the need for withIO for output.

The commits should be pretty self contained so probably best to go through that each of those. If you like the direction I can keep going and try to get the input converted to an optional option to the Game constructor.

drewish · 2017-11-02T03:13:19Z

lib/scottkit/compile.rb

@@ -25,17 +23,17 @@ def initialize(game, filename, fh = nil)
      end

      # Compiles the specified game-source file, writing the resulting
-      # object file to stdout, whence it should be redirected into a
+      # object file to an IO string, whence it should be redirected into a


oh this should be IO stream

drewish · 2017-11-02T03:14:54Z

lib/scottkit/game.rb

+      @output = options[:output] || $stdout
+    end
+
+    def puts *args


This should probably be #:nodoc: right?

I don't know about :nodoc -- I've abandoned the idea of using any of the inline documentation things, as I don't think anyone ever actually uses the resulting documents. See #19.

MikeTaylor

Remind me, what is the difference in Ruby between making a method public (as in your commit) and making it private but the declaring it public after all (as in my original)? It's been a while since I read about how this stuff works in Ruby. Or is this just a style issue?

MikeTaylor · 2017-11-02T11:37:28Z

lib/scottkit/game.rb

+    end
+
+    def puts *args
+      output.puts(*args)


I'm not sure about overriding print in game.rb. Isn't the more honest approach you used in the previous commit also suitable here?

Oh ... but you're not overriding print, are you? You're just defining a method with the same name as the underlying primitive that it wraps -- so the invocation changes from print(x) to @game.print(x). OK, that makes perfect sense. Carry on :-)

Yeah kind of funky to have to do this but it's either this or passing the output object down to the instructions. This seemed cleaner and having it centralized would give you one place to hook in a driver.

Yep, this is good.

MikeTaylor

Overall, the approach for compilation looks good to me. I think that play should follow the same model, though, and use explicit input and output parameters, rather than relying on a new :output option. We want the compilation and playing APIs to be compatible, right?

MikeTaylor · 2017-11-02T11:40:39Z

test/test_save.rb

    game.load(IO.read("games/test/crystal.sao"))
-    withIO(StringIO.new("save game\nTMP"), File.new("/dev/null", "w")) do
+    withIO(StringIO.new("save game\nTMP"), $stdout) do


Why are you no longer discarding the output of play here? We don't want that output, just the save file that is generated along the way.

This change I could probably back out. I'd started out trying to see if I could totally remove the need for withIO but left it here to pass in the save game command. Since withIO requires both params so I had to pass something in... the output is still being discarded though via the output string instead of /dev/null.

I think we probably could get rid of withIO here by just loading the game file into a StringIO object and then appending "save game\nTMP". I'll give it a try and see if that end up looking cleaner.

I don't see how writing the output of play to $stdout can discard it -- surely it will appear on standard output?

(Probably the Ruby library provides a null-file object somewhere, which exists to discard output; that would really be the best thing to use here.)

You know, it's kind of cool that something as trivial as withIO lets you do so many things :-)

there's nothing going to $stdout because it's getting sent to the :output stream instead. $stdout is only used when nothing is provided. another option would be to modify withIO to only assign non-nil values.

drewish · 2017-11-02T12:52:29Z

Remind me, what is the difference in Ruby between making a method public (as in your commit) and making it private but the declaring it public after all (as in my original)?

I think the end result is the same but just declaring it public to start results in less surprises. I remember reading through the code the first time and thinking "how is this working if these a private?" before coming across the access change later. Since they ended up public it seemed better to make the interface clearer from the start.

MikeTaylor · 2017-11-02T12:55:49Z

Cool, thanks for clarifying that. I agree, the way you have it is better.

drewish · 2017-11-02T13:05:05Z

I'm not sure if the other comments answered this or not:

Overall, the approach for compilation looks good to me. I think that play should follow the same model, though, and use explicit input and output parameters, rather than relying on a new :output option. We want the compilation and playing APIs to be compatible, right?

I'm not sure I understand this. Play should be using the :read_file and :output options. Or are you saying that the compilation should stop sending stuff to $stdout and use the :output option instead?

MikeTaylor · 2017-11-02T13:09:28Z

Sorry, this has got complicated because of all the different comment-threads!

All I am saying is that if the compilation API is now compile_to(stream), then surely the playing API should be play_to(stream) as well, instead of using an output stream hidden in the options block?

But I do see that there is then an asymmetry with input. But remember that :read_file is not just providing an input source: it's a bit more specific than that, it's saying "take the initial set of in-game commands from here, then continue as normal". (Its purpose is to allow you to replay partially-completed solutions, then continue from there manually.) So it's not really the counterpart of your :output.

drewish · 2017-11-04T18:39:03Z

That last comment makes sense. I hadn't really understood that read_file was just initial commands and then the input was consumed after that. With that in mind I think an :input option would make sense.

drewish

I realized I had misunderstood the last comment. I think the nicer thing about the compiler is that it's pretty distinct from the game (I'd actually make it more of an explicit split and just have the compiler take an options hash rather than relying on the game's options). If the player was also a distinct object that captured the instructions' output then having a play_with(input, output) method would make a ton of sense. But since the play methods are tangled up with the Game object I don't think it makes sense to inject the input and output through the play methods.

But even with that misunderstanding I think It was a good push to add an input option. It lets us remove the last usages of withIO.

drewish · 2017-11-04T19:26:06Z

lib/scottkit/game.rb

-    def compile_to_stdout(filename, fh = nil)
-      compiler = ScottKit::Game::Compiler.new(self, filename, fh)
-      compiler.compile_to_stdout
+    def compile(out, filename, fh = nil)


I'm not in love with this argument ordering but it felt like the least bad option considering the optional fh argument. If it was me I'd just ditch this method and let the callers instantiate and call the compiler themselves.

If it was me I'd just ditch this method and let the callers instantiate and call the compiler themselves.

I'm not following. What would that look like?

I'll respond on the main thread to keep things centrally visible.

drewish · 2017-11-05T14:36:31Z

There's only a couple of places where the compiler is used, in some tests and then in the scottkit binary. So it would be simple enough to just switch those to the long way

when :play_from_source
  compiled_game = StringIO.new
  compiler = ScottKit::Game::Compiler.new(game, ARGV[0])
  if !compiler.compile_to(compiled_game)
    $stderr.puts "#$0: compilation failed: not playing"
    exitval = 1
  end
  if exitval == 0
    game.load(compiled_game.string)
    play(game)
  end
when :compile
  compiler = ScottKit::Game::Compiler.new(game, ARGV[0])
  if !compiler.compile_to($stdout)
    $stderr.puts "#$0: compilation failed"
    exitval = 1
  end

The thing that seems confusing to me about having game.compile is that it doesn't compile and load the results into the game object. The comments around the compilation methods seem to hint at this:

    # Compiles the specified game-source file, writing the resulting
    # object file to stdout, whence it should be redirected into a
    # file so that it can be played.  Yes, this API is sucky: it would
    # be better if we had a simple compile method that builds the game
    # in memory in a form that can by played, and which can then also
    # be saved as an object file by some other method -- but that
    # would have been more work for little gain.

The Game seems like it wants to be the in memory structure but compiler bypasses it in favor of its own structs.

This PR's grown big enough so I'm not looking to increase the scope any more, just pointing out things have have jumped out at me. If they're things you're interested in changing I'm happy to help.

drewish · 2017-11-12T21:53:49Z

@MikeTaylor Just wanted to check in with you and see if this is something you're interested in pursuing or not. If not are there commits that you'd accept? I'm happy to just keep it in my fork but would love to land any changes that make sense to you.

MikeTaylor · 2017-11-12T22:06:44Z

I agree there is a issue here, but it needs thought and I won't be able to give it any for 72 hours. If you can wait, that'd be best for me.

drewish · 2017-11-12T22:43:48Z

No rush at all. Just wanted to give you an easy way to say no if it wasn’t a direction that makes sense to you.

drewish mentioned this pull request Oct 30, 2017

Refactor game loop to allow easy external use #16

Merged

drewish commented Nov 2, 2017

View reviewed changes

MikeTaylor reviewed Nov 2, 2017

View reviewed changes

drewish force-pushed the instructions-print-via-game branch 3 times, most recently from d26898f to 8e389b5 Compare November 4, 2017 19:30

drewish commented Nov 4, 2017

View reviewed changes

drewish force-pushed the instructions-print-via-game branch from 8e389b5 to 78d2c29 Compare November 8, 2017 15:45

drewish added 6 commits December 4, 2017 20:34

Remove execute permissions

e0eb026

Make dputs public and more explicit

c42cd47

Take compiler output as argument, make public methods public to start

ebe2aed

Add output option for capturing game output

3f02d02

Add an input option as well

73fbb03

Remove withIO

7dba848

drewish force-pushed the instructions-print-via-game branch from 78d2c29 to 7dba848 Compare December 5, 2017 03:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Instructions print via game #32

Instructions print via game #32

drewish commented Oct 30, 2017

drewish Nov 2, 2017

drewish Nov 2, 2017

MikeTaylor Nov 2, 2017

MikeTaylor left a comment

MikeTaylor Nov 2, 2017

MikeTaylor Nov 2, 2017

drewish Nov 2, 2017

MikeTaylor Nov 2, 2017

MikeTaylor left a comment

MikeTaylor Nov 2, 2017

drewish Nov 2, 2017

MikeTaylor Nov 2, 2017

drewish Nov 4, 2017

drewish commented Nov 2, 2017

MikeTaylor commented Nov 2, 2017

drewish commented Nov 2, 2017

MikeTaylor commented Nov 2, 2017

drewish commented Nov 4, 2017

drewish left a comment

drewish Nov 4, 2017

MikeTaylor Nov 5, 2017

drewish Nov 5, 2017

drewish commented Nov 5, 2017 •

edited

Loading

drewish commented Nov 12, 2017

MikeTaylor commented Nov 12, 2017

drewish commented Nov 12, 2017

Instructions print via game #32

Are you sure you want to change the base?

Instructions print via game #32

Conversation

drewish commented Oct 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeTaylor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeTaylor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewish commented Nov 2, 2017

MikeTaylor commented Nov 2, 2017

drewish commented Nov 2, 2017

MikeTaylor commented Nov 2, 2017

drewish commented Nov 4, 2017

drewish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewish commented Nov 5, 2017 • edited Loading

drewish commented Nov 12, 2017

MikeTaylor commented Nov 12, 2017

drewish commented Nov 12, 2017

drewish commented Nov 5, 2017 •

edited

Loading