Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Window command line generation patches #762

Merged
merged 3 commits into from Dec 30, 2012

Conversation

Projects
None yet
2 participants
Contributor

jojje commented Dec 30, 2012

Two commits.

  1. The fix for quoting and its associated test updates.
  2. A regression test to avoid single-quoted arguments from creeping in again, in the future (as requested).

@myronmarston myronmarston and 1 other commented on an outdated diff Dec 30, 2012

spec/autotest/rspec_spec.rb
@@ -28,9 +28,15 @@
@to_test = files.map { |f| File.expand_path(f) }.join ' '
end
+ it "uses double quotes for windows compatibility" do
+ actual_command = rspec_autotest.make_test_cmd(@files_to_test)
+ expected_pattern = /^[^\']+$/
+ actual_command.should match(expected_pattern)
+ end
@myronmarston

myronmarston Dec 30, 2012

Owner

I think this could be expressed in a simpler fashion:

command = rspec_autotest.make_test_cmd(@files_to_test)
command.should include('"')
command.should_not include("'")

The regex seems more convoluted and doesn't actually verify that double quotes are included....which seems kinda important.

@jojje

jojje Dec 30, 2012

Contributor

I agree, proper use of matches is key to expressiveness and when you see people not exploiting the full vocabulary of your test framework it only indicates one thing; lack of insight :)

My reasoning for not checking for double quotes was that I was thinking a bit non-TDD, namely "what if he decided in the future to no longer need quoting, then the test would break at that point", instead of the proper notion: "how is the code supposed to work NOW". I'll remedy it as suggested.

Owner

myronmarston commented Dec 30, 2012

BTW, please don't open a separate pull request for additional changes...it makes more work for us to have to track multiple pull requests than just the one. You can always force push if you want to rebase and squash things down into fewer commits.

Contributor

jojje commented Dec 30, 2012

Ok, thanks for that useful advice, appreciate it. Still in a learning mode for proper collaboration on this site.

Owner

myronmarston commented Dec 30, 2012

Ok, thanks for that useful advice, appreciate it. Still in a learning mode for proper collaboration on this site.

No worries...I'm glad to see you took a stab at diving right in and contributing :).

Contributor

jojje commented Dec 30, 2012

On a side note: Did a test on the force update approach as you mentioned by amending the commit message, to see how github and travis react, and indeed, Github allows rewriting the history (not that I doubted you), but Travis seems to regard commit amendment (changing commit message only) as a change that requires a complete rebuild and re-test.

Perhaps something I should raise to github as an enhancement request, esp. since projects such as yours take 18 minutes to re-test (.. jruby :< ..).

@myronmarston myronmarston added a commit that referenced this pull request Dec 30, 2012

@myronmarston myronmarston Merge pull request #762 from jojje/fixes
Window command line generation patches
34577db

@myronmarston myronmarston merged commit 34577db into rspec:master Dec 30, 2012

1 check passed

default The Travis build passed
Details

@myronmarston myronmarston added a commit that referenced this pull request Dec 30, 2012

@myronmarston myronmarston Add changelog entry for #762.
[ci skip]
cc2e56f
Owner

myronmarston commented Dec 30, 2012

Thanks @jojje!

Travis seems to regard commit amendment (changing commit message only) as a change that requires a complete rebuild and re-test.

Well, a new commit is a new commit. I suppose travis could look at the SHA of the git tree itself (not taking into account the commit sha) but I've never thought of this as a problem.

Perhaps something I should raise to github as an enhancement request, esp. since projects such as yours take 18 minutes to re-test (.. jruby :< ..).

You can certainly bring it up, but I'd recommend bringing it up with the travis folks. It would be a change on their side if it got implemented.

@jojje jojje deleted the unknown repository branch Feb 14, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment