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

Prepare for RSpec 3.x #127

Closed
wants to merge 1 commit into from
Closed

Prepare for RSpec 3.x #127

wants to merge 1 commit into from

Conversation

petergoldstein
Copy link
Contributor

  1. Changed to RSpec expect syntax.
  2. Replaced 'be_true' and 'be_false' matchers with 'be true' and 'be false'

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 602dd7c on petergoldstein:feature/prepare_for_rspec_3 into 36c6a49 on floere:master.

@floere
Copy link
Owner

floere commented Jan 5, 2014

Hi Peter!

Thanks for the PR. Some issues:

Cheers!

@petergoldstein
Copy link
Contributor Author

Hi @floere

  1. I'm not certain what you mean when you say that my search and replace seems to have matched too many instances. Can you provide some examples?
  2. Regarding the matchers, be true and be false are specifically not equivalent to be_truthy and be_falsy. As discussed in the linked issue, this was the whole point of renaming be_true andbe_false` in the first place, to address this ambiguity.

Phony.plausible? and other methods under test are specifically intended to return boolean values, which means that using be true and be false is the more restrictive and correct test. For example, if this method suddenly started returning 7 and nil for previously true and false return values, I'd consider this a bug. And accordingly it would fail the specs in this PR. If, however, I had used be_truthy and be_falsy then the specs would not fail under this changed behavior. So in my view the correct specs will not use the be_truthy and be_falsy matchers

@petergoldstein
Copy link
Contributor Author

@floere Ok. Give me a few minutes to rebase and get your latest changes, then I'll push updated code.

@floere
Copy link
Owner

floere commented Jan 5, 2014

Thank you very much!

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 9f00482 on petergoldstein:feature/prepare_for_rspec_3 into 7eef1fa on floere:master.

@petergoldstein
Copy link
Contributor Author

@floere Not sure how you're feeling about the be true/be false based on my reply above. For the reasons described there I left in be true and be false in preference to using be_truthy and be_falsy

@floere
Copy link
Owner

floere commented Jan 5, 2014

@petergoldstein No worries. I extremely rarely explicitly test for true/false since return values of ? methods in Ruby only need to evaluate true or false, as they are not supposed to be used any other way (imho – I can't find a reference for it currently). But give me a day to get used to it, and also the hard to digest expect syntax (more parentheses, thing under test not leftmost and tests thus easily scannable) – yes, I am a bit picky ;)

@petergoldstein
Copy link
Contributor Author

@floere I have to admit I'm not exactly nuts about the changed syntax, although the underlying motivation to keep Object clean makes sense. Anyway, sit on it for a day or two and let me know if you want to discuss any changes. Thanks.

@floere
Copy link
Owner

floere commented Jan 5, 2014

@petergoldstein Thanks for your understanding ;)

@petergoldstein
Copy link
Contributor Author

Rebased for recent changes. Updated newly added specs to expect syntax.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) when pulling aad15bb on petergoldstein:feature/prepare_for_rspec_3 into e795779 on floere:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) when pulling 2050016 on petergoldstein:feature/prepare_for_rspec_3 into f408df6 on floere:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0%) when pulling 0f5c750 on petergoldstein:feature/prepare_for_rspec_3 into 5a931cd on floere:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) when pulling 9f8cf66 on petergoldstein:feature/prepare_for_rspec_3 into c492738 on floere:master.

@petergoldstein
Copy link
Contributor Author

@floere Let me know what you're thinking here. I've rebased to latest changes, so this is ready for merge. But if you're not planning to merge it in, I'd prefer to just close it at this point.

@floere
Copy link
Owner

floere commented Feb 13, 2014

Hi @petergoldstein,

First of all, let me say that I am terribly sorry for not deciding earlier. That was careless and not very respectful of your time!
After good experiences with bacon, I am strongly considering switching to that test framework. I do prefer leaner libraries. So sadly, I'll have to close this pull request, but let me say that I really appreciate your work and moving it with master, which is why I added you under contributors, here: https://github.com/floere/phony/wiki.
I hope to be able to shout (unsure what the American equivalent would be here) you a beer some time in the future!

All the best,
Florian

@floere floere closed this Feb 13, 2014
@petergoldstein
Copy link
Contributor Author

Hey @floere,

No worries. I look forward to getting that beer with you sometime.

Best,

Peter

@floere
Copy link
Owner

floere commented Feb 13, 2014

Great, and likewise! All the best :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants