search

aroberge / reeborg

Enhanced Karel-the-robot clone
http://reeborg.ca/reeborg.html
Other
47 stars 36 forks source link

fix for broken repeat statement #451

Closed schellenberg closed 5 years ago

schellenberg commented 5 years ago

The only problem appears to be an accidental comment on the variable declaring the number of iterations.

schellenberg commented 5 years ago

In addition to the repeat statement fix, I wrote a quick way to switch to the correct world when loading a save file. Right now, it is fragile, in that it will only load the world correctly if the file name is exactly the same as expected. Might be good to improve this, so that if a student saves multiple versions of a file, Reeborg can intelligently guess what Step 1(1).py means...

aroberge commented 5 years ago

Hi Dan, Thanks for looking into this. The comment was inserted when, at the request from another user, I finally "broke down" and removed the previous restriction on "n" in repeat n: to be identifiable as an integer - hence, a variable could be used. I thought it passed all the tests when I did this but obviously, it did not.

I'll try to have a look at it tomorrow.

André

On Thu, Apr 18, 2019 at 3:58 PM Dan Schellenberg notifications@github.com wrote:

The only problem appears to be an accidental comment on the variable declaring the number of iterations.

You can view, comment on, or merge this pull request online at:

https://github.com/aroberge/reeborg/pull/451 Commit Summary

  • fix for broken repeat statement

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/aroberge/reeborg/pull/451, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEZXQQATRFR2IXESXJA6RDPRDAFBANCNFSM4HG7QSWQ .

aroberge commented 5 years ago

Sorry, it took longer for me to look at this - the flu took a lot more out of me than I expected.

I fixed the repeat n problem by reverting to the old way; like you had done and submitted in your pull request. Along the way I fixed all the integration tests that had been broken when I attempted to clean them up after I got a new computer and cloned the repo. This is likely why I never caught the silly mistake about the broken repeat n.

Regarding the other fix that you propose: I am going to look at this in a completely separate commit. I think that there is a way to do this which would work, regardless of the name of the file. I will create a separate issue to track this.