Fix restart

[akaneko3]

Description

I modified 'restart' command to run on Windows.

[deivid-rodriguez]

Thanks for the fix, the only thing I don't like is how the test code exactly duplicates the implementation. What if we always added ruby in front of the command?

[akaneko3]

On Windows, Kernel.exec('foo.rb') is not running. We have to change this to Kernel.exec('ruby foo.rb'). Accordingly, the expected output of restart is changed on Windows.

What if we always added ruby in front of the command?

I don't know because I don't have a Linux environment.

[akaneko3]

I fix execute method in restart.rb and test methods in restart_test.rb, always add Ruby interpreter path. How about this?

[deivid-rodriguez]

Yes, definitely! The reason I asked for this is that this problem is not specific to Windows. If you run ruby myscript.rb where myscript.rb has no shebang or is not executable, restart will fail in Linux too. So I think it makes more sense to just always add ruby before the command when restarting.

Can we add a separate test with the assertion that we want the ruby executable before the command? (def test_restart_adds_ruby_before_the_command or something like that). Due to the bad shape of the current restart tests (not really restarting but just mocking the exec call and doing assertions on it), it won't be a regression test since it currently passes, but at least it might help to document that prepending ruby is necessary (until we can improve the tests).

What do you think?

[akaneko3]

Sorry, It seems that the purpose of the correction was not conveyed.

If you run ruby myscript.rb where myscript.rb has no shebang or is not executable, restart will fail in Linux too. So I think it makes more sense to just always add ruby before the command when restarting.

I understood that. Therefore, I fix execute method always add ruby before the command when restarting. I confirmed that this fix works on Linux.

Please check the latest commit.

[deivid-rodriguez]

Yes, I checked the last fix and I like it. I just wanted an extra test to serve as documentation for the fix, but I guess we can live without it.

Just two more things before merge:

• Can you change the commit that touches $ARGV to instead require 'English' on top of the file. I use $ARGV everywhere else and, although it doesn't help with readability in this case, I prefer to use it consistently.

• Can you squash all commits down to two (one for the $ARGV fix and one for the ruby fix) and include a change log entry with each of them?

[akaneko3]

Just two more things before merge:

• Can you change the commit that touches $ARGV to instead require 'English' on top of the file. I use $ARGV everywhere else and, although it doesn't help with readability in this case, I prefer to use it consistently.

• Can you squash all commits down to two (one for the $ARGV fix and one for the ruby fix) and include a change log entry with each of them?

OK, I fixed it as you suggested.

[deivid-rodriguez]

Looks great! Just missing a rebase and a changelog entry with each commit in the "Fixed" section of the changelog.

[akaneko3]

Should I add the following sentences to CHANGELOG.md ?

Fix_restart (Unreleased)

Fixed

• Require English in restart.rb for use $ARGV.
• Modified restart that Kernel.exec always call ruby.

[deivid-rodriguez]

For the ARGV bug you can say:

• restart command crashing in certain cases because of a missing require 'English' (Fix restart #321, @akaneko3).

For the other one:

• restart command crashing when debugged script is not executable or has no shebang (Fix restart #321, @akaneko3).

[akaneko3]

Thanks! May I write it to CHANGELOG.md and commit it?

[deivid-rodriguez]

Of course. If you can add each sentence to each commit (keeping the PR with two commits), that's preferred. Also, don't forget to rebase 😉

[akaneko3]

HI, Deivit, I fixed CHANGELOG.md and rebased repository a few days ago.
Are you possibly busy?
Please merge PR, I don't mind if you merge in your free time.

[deivid-rodriguez]

Sorry, I forgot... Last comments, could you use single backquotes instead of triple backquotes like it's done in the other entries in the change log? Also, regarding the commit messages, the correct wording for the second commit would be "Modified 'restart' Kernel.exec to always call 'ruby'" instead of "Modified 'restart' Kernel.exec is always call 'ruby'".

[akaneko3]

OK, I fixed CHANGELOG.md and commit message.

[deivid-rodriguez]

Thank you so much!

[akaneko3]

Thank you for merge and advising me a lot!