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

Function same as with, but returns the incoming object #174

Closed
wants to merge 1 commit into from
Closed

Function same as with, but returns the incoming object #174

wants to merge 1 commit into from

Conversation

christoph-frick
Copy link
Contributor

@christoph-frick christoph-frick commented Nov 1, 2015

My usecase for with is >80% to (ab)use it as some sort of builder. Since I end up putting close to always (return) it at the end of my closure: doto tap is a slight variation of with, that just returns the incoming object. doto is a loanword from clojure, tap is a loanword from Ruby (see comments below) where it does the same and I could not find a naming scheme for groovy, that would not involve renaming with (which is impossible).

@blackdrag
Copy link
Contributor

without saying we should do this, there is at least one part that needs improvement. Your tests do not check the returned value at all. Since that is what is one deciding factor to have this method at all, I would say testing the returned value is absolutely required

@christoph-frick
Copy link
Contributor Author

yes, there currently is only one "happy path" test and the rest is the with test adopted. I'll add more tests once, there is some vibe to add this function.

@shils
Copy link
Member

shils commented Nov 11, 2015

A similar method named tap exists in Ruby, and was implemented as an extension method here. As far as a vibe to add this, you'd probably get more feedback if you posted on the users mailing list.

@jwagenleitner
Copy link
Contributor

Just wanted to note that GROOVY-3976 is an open JIRA related to this. FWIW, I kind of agree with the reporter and would be surprised if many rely on the current behavior of with and would be more in favor of a breaking change the next major release than a new yet similiar method. If it's a new method though, I like tap over doto.

@christoph-frick
Copy link
Contributor Author

Well I don't do ruby, so I have no opinion on the naming; if ruby is the source to look for names, I happily change it to tap. But I am totally against changing with. I rarely use with without return, but I use it.

@paplorinc
Copy link

Nice change, wanted to do this myself also.
If we're changing the with instead (I would vote for that, as I find doto and tap unintuitive), I suggest we do the same for each also.

Also, please check for NullObject as well, as it might act as an alternative null check.

Btw, wouldn't it be simpler to implement it as

def doto(self, closure) { self?.with(closure); self }

(with extension methods, of course)

@solvingj
Copy link

@christoph-frick Thanks for putting this together, it's been an outstanding request for 6+ years. There seems to be enough support for this feature, and only the naming discussion is holding it up. If you don't mind changing the method name to tap, then it seems like this could be merged.

@dovchinnikov
Copy link
Contributor

IMHO doto is ugly name.
I vote for apply.

@christoph-frick
Copy link
Contributor Author

@dovchinnikov apply means at least in python, javascript, clojure: take a function and a list and run that function with each element of the list as argument to that function.

@dovchinnikov
Copy link
Contributor

@christoph-frick It doesn't mean anything in Groovy :) Groovy already has each.
Btw Kotlin uses apply.

@solvingj
Copy link

"apply" makes a lot of sense to me. Now that I know that Kotlin has the exact same feature named apply, it seems to me to be the obvious choice.

@paulk-asert
Copy link
Contributor

Hi Christoph, I think this is useful and we should go ahead - we mostly seem to be stalled on naming. Any chance you can rename to 'apply' to match Kotlin's naming scheme and add the tests that Jochen suggested and rebase? Once done I'll send an email to the users mailing list to get any final thoughts on naming alternatives (a final rename is easy if needed) and I can help get this through into 2.5. We hope to release a beta shortly.

If you also wanted to add in some doco, feel free otherwise I'll do that. [search for 'Using with() for repeated operations on the same bean' within style-guide.adoc to find the right place and no I don't know why it is only there but that will do].

@christoph-frick
Copy link
Contributor Author

I am really sorry, for causing so much turmoil. That patch was not considered
worthy for a year now and quite honestly: I don't want my name to be associated
with a function apply, that does not do, what is expected of it
(https://en.wikipedia.org/wiki/Apply).

For me, Groovy naming is odd, since I am no Ruby developer in the first place,
but I would consider it the Groovy heritage for picking a name for a function.
So tap makes no sense to me -- same as e.g. inject makes no sense to me
(fold, reduce do) -- but if Ruby developers know that term, I am willing
to learn.

Since to me Kotlin has its influence from Groovy, "back-porting" a Kotlin name
-- that feels absolutely wrong to me -- is not fine with me. So I'd rather
close this ticket now. So one of the proponents for apply can do their own
patch for fame and glory ;)

@paulk-asert
Copy link
Contributor

paulk-asert commented Nov 7, 2016

Hi Christoph, I was planning on getting more feedback on the name in the mailing list. I was just suggesting apply as the holding pattern since there seems to be as much or more acceptance of that name than doto. Kotlin doesn't seem to have confusion between the specific with-like apply and the traditional functional version but that is one of the points that would be debated in the mailing list. I understand if you want to withdraw but after the debate, the name may still end up being doto, who knows.

Would any of these names work for you: withThis, auto, autoWith, asThis?

P.S. inject and collect come from Smalltalk not Ruby.

P.P.S. and it's not the case that your PR was ever considered 'not worthy', it's just that naming is hard and we prefer to delay and get right rather than rush and make further naming even harder down the track.

@christoph-frick
Copy link
Contributor Author

christoph-frick commented Nov 8, 2016

@paulk-asert No hard feelings here. "not worthy" as in: it seems not enough a common problem.

I have now rebased the patch against upstream, renamed to function to tap (which was the first "leader" in the name-race). There is already a test for assertSame.

As for the name, feel free to settle on one with the groovy elders (but not apply ;))

@paulk-asert
Copy link
Contributor

Thanks! That looks good. I'll send an email to the users list and ask for feedback on the name.

@christoph-frick christoph-frick changed the title add doto: same as with, but returns the incoming object Function same as with, but returns the incoming object Nov 8, 2016
@jameskleeh
Copy link
Contributor

+1 for withThis

@paulk-asert
Copy link
Contributor

Hey Christoph, the general consensus seems to be tap plus with(true) as an alternative option (basically a variant with an extra "boolean returning" parameter). Plus we need a little bit of asciidoc doco. Are you happy to do either of these or are you happy for me to take the PR as is and make the necessary tweaks?

@christoph-frick
Copy link
Contributor Author

@paulk-asert I have added some documentation at the spot you suggested earlier. Feel free to take over from here - or let me know, if something is missing I can do.

@graemerocher
Copy link
Contributor

I'm for tap. I think withThis is ugly, with(true) seems like a hack to change the behaviour of an existing method.

@paulk-asert
Copy link
Contributor

@christoph-frick thanks, I'll take it from here - will let you know if there are any problems

@graemerocher yes, with(true) is there for users that find tap a totally foreign name and to help link normal with and tap conceptually. Using tap will be preferred because we can pass along better typing information with its signature.

@asfgit asfgit closed this in bc90dd9 Nov 15, 2016
@paulk-asert
Copy link
Contributor

Proposed PR and some minor tweaks applied. Thanks everyone for the contribution, discussion and patience.

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