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

Add an overload of FlatMapOps#>> that gives control on the evaluation… #492

Merged
merged 1 commit into from
Sep 9, 2015

Conversation

julienrf
Copy link
Contributor

… strategy of the second action

@codecov-io
Copy link

Current coverage is 63.74%

Merging #492 into master will decrease coverage by -0.05% as of e9f63c9

@@            master    #492   diff @@
======================================
  Files          154     154       
  Stmts         2359    2361     +2
  Branches        66      66       
  Methods          0       0       
======================================
  Hit           1505    1505       
  Partial          0       0       
- Missed         854     856     +2

Review entire Coverage Diff as of e9f63c9

Powered by Codecov. Updated on successful CI builds.

@ceedubs
Copy link
Contributor

ceedubs commented Aug 29, 2015

On Gitter there was some discussion on whether to overload method names or use a consistent convention like an Eval suffix (such as pureEval) when an Eval alternative is provided. I like the idea of consistency, but that wouldn't work in the case of a symbolic operator like this one.

👍 from me, but I'm hoping @travisbrown can weigh in, because I remember he mentioned being in favor of not overloading.

@julienrf
Copy link
Contributor Author

I’m fine with adding a suffix like Eval but we need to find a real name for that method :)

@travisbrown
Copy link
Contributor

This SO answer is a good summary of some of the reasons to avoid overloading. I personally think this is more important for things like pure—some of the big problems with overloading (eta expansion, selective imports) just aren't as relevant for instance methods, and operators like this seem to be one of the places where they're easier to justify.

Even here though it still introduces some annoying questions. Is Eval.Unit >> Eval.now(Eval.True) an Eval[Eval[Boolean]] or just an Eval[Boolean]? What if you actually want it to be the former?

@tpolecat
Copy link
Member

In this case I think the subtlety introduced when F is Eval is sufficient reason to disambiguate.

@ceedubs
Copy link
Contributor

ceedubs commented Aug 29, 2015

Thanks @travisbrown. You have me convinced that disambiguating might be a good idea.

@julienrf
Copy link
Contributor Author

Hi,

I totally agree on the issues of overloading. I will be happy to remove my overload and to introduce two new methods, one that is an alias for the current >> and the other, suffixed with Eval, with the new signature … as soon as we will have found a nice name :)

What about discardAndThen? andAfter? Other suggestions?

@julienrf
Copy link
Contributor Author

julienrf commented Sep 7, 2015

Hi guys,

I decided to go with andAfter since monads are essentially about control flow. Tell me what you think :)

@non
Copy link
Contributor

non commented Sep 7, 2015

Interesting. I think that's a good name, but it made me think of followedBy which might be better. What do you think?

@julienrf
Copy link
Contributor Author

julienrf commented Sep 8, 2015

I’m fine with followedBy too. What do others think?

@julienrf
Copy link
Contributor Author

julienrf commented Sep 8, 2015

I updated the PR to use followedBy.

@non
Copy link
Contributor

non commented Sep 8, 2015

This looks good to me. 👍

Did you want to create an overload for >> so you can choose to pass an Eval[F[B]] as well as an F[B]? If so, I would support adding it.

@julienrf
Copy link
Contributor Author

julienrf commented Sep 8, 2015 via email

@non
Copy link
Contributor

non commented Sep 8, 2015

👍 from me then. Thanks for sticking with this! :)

@milessabin
Copy link
Member

👍

milessabin added a commit that referenced this pull request Sep 9, 2015
Add an overload of FlatMapOps#>> that gives control on the evaluation…
@milessabin milessabin merged commit 6c91384 into typelevel:master Sep 9, 2015
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.

7 participants