-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
Should use Object#public_send
instead of Object#send
.
#28
Comments
I agree with you, but am a little reluctant to make this change, since it will be a "breaking" one and the Ruby and RSpec precedent is to avoid those at pretty much all costs. Any of the few watchers want to chime in on this? |
The important thing isn't to avoid these sorts of changes; it's to follow SemVer so that users are aware of when new releases have breaking changes. In RSpec we usually hold changes like this off until the next major version, or introduce a config option that enables it (and defaults to For rspec-its, I think a config option is kinda overkill (and there's no easy way to hook into One issue to bear in mind is Ruby 1.8.7 -- it lacks |
Object#public_send
instread of Object#send
.Object#public_send
instead of Object#send
.
My vote would be for dropping 1.8.7 support (since it is EOL and unsupported), but that's just my 2¢. I fixed the failed build over in #33 without removing support for 1.8.7, but the code would obviously be a lot simpler without the |
@palfvin I would love to see this merged in for the next major release. This was weird gotcha for us that felt like unexpected behavior. Thanks for all your great work! 😸 |
@tonyta Since I'm not sure when/if there will be another "major" release of this little gem, or at least one prior to RSpec 4, would a config option help you out in the meantime? |
What about to simply be more opinionated. The use of I do not have a real problem to force better design on the users of my tools. Especially on a development tool that will NOT be part of production a bigger change might be justifiable. |
The question I have is whether #33 as it stands right now is considered to be a breaking change? I don't think it is, because it still supports the fallback to If #33 is considered to be a breaking change, then I vote we drop support for 1.8.7 and simplify the code by not supporting the fallback. In this case I can update #33 and I see no reason to wait for RSpec 4 to release a new major version of If #33 is not considered to be a breaking change, then I think #33 should be merged and released as a minor version bump. In this case, a new PR to drop support for Ruby 1.8.7 should be proposed and the decision for whether to couple the major version bump of |
I just got bitten by this issue as well. I expected the following to be equivalent: its(:foo) { should eq("bar") }
it { expect(subject.foo).to eq("bar") } I think it is only in exceptional circumstances that you want to test private methods, so breaking compatibility isn't a bad thing. |
This seems like a bug in the Since this behaviour is not documented, do we have to consider it as a breaking change ? I wonder if a configuration option could be added to use the previous functionality. I guess it's down to your interpretation of "breaking change". Whatever the case, I would really like to see this changed ! Would you accept a PR ? |
It is a breaking change, yes. #33 (comment) If you are up to helping with updating |
@pirj Yes, saw that comment, but I kind-of disagree with it. That's okay though - I don't disagree strongly, and it's not my decision anyway. :-) I think I could find some time to get involved with RSpec 4 compatibility. Do you have a roadmap/list of known issues/anything else ? I should be able to find some time next weekend if not during the week. |
Off the top of my head:
Optionally update rake and cucumber to the latest. There is a fistful of pull requests/issues. Frankly, I didn't look closely. That should do it. Really appreciate it if you could take this over. |
@pirj Okay, thanks a lot. I can't make any promises but it would be nice to see Enjoy the rest of the weekend. |
I created #33 back when I first found this issue, but it fell through the cracks. I'd love to see it merged one day, so thanks for jumping in to add your voice to this one. |
I just found that
rspec-its
is able to call private getters on subject, this is probably not intentional and a side effect from usingObject#send
instead ofObject#public_send
.rspec-its/lib/rspec/its.rb
Line 115 in 38fdc24
The text was updated successfully, but these errors were encountered: