Implement respond_to? in module Monad (Fresh one)
#10
Conversation
sclinede
force-pushed
the
master
branch
6 times, most recently
from
c901dc3 to
5b4f598
Compare
sclinede
changed the title
respond_to? in module Monadrespond_to? in module Monad (Fresh one)
|
This should fix errors found for now. @tomstuart WDYT ? |
|
Thank you for this! I intend to merge it, but I’ll leave a couple of comments on the code first. |
.travis.yml
Outdated
| - 2.1.2 | ||
| script: bundle exec rspec | ||
| before_install: | ||
| - gem update --system 2.4.8 |
There was a problem hiding this comment.
I gather that this is to work around the incompatibility between the Bundler and RubyGems versions on Travis CI, but we don’t need to update RubyGems itself to fix that, only Bundler. So I don’t think we need to gem update --system here at all.
lib/monads/monad.rb
Outdated
| end | ||
|
|
||
| def respond_to_missing?(method_name, include_private=false) | ||
| super || within do |value| |
There was a problem hiding this comment.
As a matter of principle, I think we this should be … || super rather than super || …. We should give the monad a chance to reply first, and only ask the superclass if necessary, rather than vice versa. (This probably doesn’t make a difference in practice — I’m thinking about educational value.)
See details here: tomstuart#4
Resolves tomstuart#2, as previous had no tests. See details in tomstuart#2 PR, breifly: Prompted by @tomstuart statement: "If this was production code, we should remember to implement #respond_to? as well."
|
Oh. I think I found were was a problem with #8. I'll add a commit to resolve it. |
|
🆙 |
lib/monads/many.rb
Outdated
|
|
||
| def self.from_value(value) | ||
| Many.new([value]) | ||
| Many.new(value.respond_to?(:to_ary) ? value.to_ary || [value] : [value]) |
There was a problem hiding this comment.
I think this is (more or less) what Kernel#Array does. Any reason we can’t have Many.new(Array(value)) here instead?
|
hmm. that's interesting. At first i didn't understand #8 and that is why I asked for more info... Since I don't have any real use case for this, I agree that in some cases new logic can cause unpredictable result, I think we should remove it from PR. |
|
Makes sense. It could always move into a separate PR in case anyone else wants to discuss/defend it there? |
|
of course. I'll create one. |
|
Looks great, merging. Thank you @sclinede for this pull request, and thank you @roryokane, @gf3 and @marcinwyszynski for your contributions! |
Implement `#respond_to_missing?` and stop using Struct
|
👍 |
|
Any chance to have this released as a new version on rubygems? We actually happily use this gem in production in codebeat |
|
I will release it shortly. I think the implementation is actually not quite right, so I’m just doing some testing and cleaning up before I make a new release. |
|
I’ve released 0.1.0 now. The sticking point was 3e0cdd7. |
|
Hey guys ^^ To shed a little light on issue #8 and pending PR #9, I've explained why flat_map doesn't do the job when there's more than one level of nesting ; to make it clearer, I'm about to make a failing test case with current codebase that'll illustrate the bug and rebase the solution on top of it to make a better PR. Once again, thanks for bringing the awesomeness of monads in Ruby (and in my coder's life ^^). |
|
There you go, just re-submitted an update PR #9 (adding a test showing the problem was still present ^^). Hope it helps, cheers. |
5 participants
Resolves #2, as previous had no tests.
See details in #2 PR, breifly:
After merging this we may close #2, #5, #7, #4