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

[5.5] Allow passing a callback to "with" #21445

Merged
merged 1 commit into from
Sep 28, 2017

Conversation

JosephSilber
Copy link
Member

Similar to tap, but returns the value from the callback.

@taylorotwell taylorotwell merged commit d3420ed into laravel:5.5 Sep 28, 2017
edmandiesamonte added a commit to edmandiesamonte/framework that referenced this pull request Oct 2, 2017
* upstream/5.5: (84 commits)
  Correct docBlock depenency on syncWithoutDetaching (laravel#21478)
  update v5.5 changelog
  allow to specify the queue while scheduling of queued jobs (laravel#21473)
  [5.5] Clear CountQuery "select" bindings since we're overriding the columns anyway (laravel#21468)
  add interface
  access pivot on resource
  update v5.5 changelog
  extract trait
  Fix docblock in Route (laravel#21454)
  fix test
  formatting
  Fix Relation::morphMap() merge (issue laravel#21457). (laravel#21458)
  extract AnonymousResourceCollection into class to allow serialization
  revert relationship limits
  Fix spelling of 'optionally'
  [5.5] Fix Collection::contains() when the found value is null (laravel#21442)
  Allow passing a callback to "with" (laravel#21445)
  [5.5] Add relation and model attributes in RelationNotFoundException (laravel#21426)
  [5.5] Make sure sql for virtual columns is added after the unsigned modifier (laravel#21441)
  vendor:publish options alphabetized (laravel#21412)
  ...
@JosephSilber JosephSilber deleted the with-callback branch October 4, 2017 14:44
@alepeino
Copy link
Contributor

alepeino commented Oct 9, 2017

I just found about this PR, and it surprised me how it was instantly merged, while #20987 from a few days earlier had been closed without a comment...

@JosephSilber
Copy link
Member Author

@alepeino the use-case described in that PR sounds like it should be using tap. This PR’s description is for returning an entirely different value from the callback.

While the code is the same, the way it was presented is different. And for someone juggling tons of PRs daily, the proper description can make all the difference.

@alepeino
Copy link
Contributor

alepeino commented Oct 9, 2017

Yes, I agree this wording is clearer, even though the use-case is evident from the test and from the comparison with a well-known library such as Lodash.

Don't get me wrong, I'm still glad the feature is merged in, it just felt a little confusing, and disappointing too, being a communication issue like you say, not having the opportunity to reply and do a better explanation.

Anyway, cheers

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.

None yet

3 participants