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 State/StateT #302

Merged
merged 3 commits into from
May 20, 2015
Merged

Add State/StateT #302

merged 3 commits into from
May 20, 2015

Conversation

ceedubs
Copy link
Contributor

@ceedubs ceedubs commented May 5, 2015

Fixes #122.

To avoid stack overflows, a couple design choices were made that make
State more complicated than it seems like it should be.

State wraps F[S => F[(S, A)]] instead of the more familiar S => F[(S, A)]. This allows for effective trampolining.

State[S, A] is a type alias for StateT[Trampoline, S, A] to avoid stack
overflows that are easy to hit if Id is used instead of Trampoline.

State is currently in its own module because it depends on Free (for
trampolining) but doesn't seem to belong in the Free module. Since it's
tough to avoid trampolining in FP Scala, we may want to reconsider
whether Free should be in core.

There is a brief ScalaDoc comment on StateT, but a proper markdown file
should probably be created. If this gets merged, I can send a follow-up
PR with some more documentation.

To avoid stack overflows, a couple design choices were made that make
State more complicated than it seems like it should be.

State wraps F[S => F[(S, A)]] instead of the more familiar S => F[(S, A)]. This allows for effective trampolining.

State[S, A] is a type alias for StateT[Trampoline, S, A] to avoid stack
overflows that are easy to hit if Id is used instead of Trampoline.

State is currently in its own module because it depends on Free (for
trampolining) but doesn't seem to belong in the Free module. Since it's
tough to avoid trampolining in FP Scala, we may want to reconsider
whether Free should be in core.

There is a brief ScalaDoc comment on StateT, but a proper markdown file
should probably be created. If this gets merged, I can send a follow-up
PR with some more documentation.
@ceedubs
Copy link
Contributor Author

ceedubs commented May 5, 2015

This is embarrassing. One of my more recent changes appears to have made the test not compile and I guess I didn't run the tests again before submitting. Will fix :(

@aryairani
Copy link
Contributor

LGTM, plus I like the runX names.

/**
* Run with `S`'s empty monoid value as the initial state.
*/
def runEmpty(implicit S: Monoid[S], F: FlatMap[F]) = run(S.empty)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing the return type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Pushed a new commit that fixes the three instances.

And also simplify the StateT.map implementation.
@ceedubs
Copy link
Contributor Author

ceedubs commented May 6, 2015

@jdegoes would you be willing to take a look at this since you provided a lot of the input on #122?

@jdegoes
Copy link

jdegoes commented May 6, 2015

Looks great (application of the state function now cannot be done recursively without passing through F at every application of state), and I like the 'safe by default' choice for State. 👍

@non
Copy link
Contributor

non commented May 20, 2015

Looks great to me. 👍

non added a commit that referenced this pull request May 20, 2015
@non non merged commit 2698226 into typelevel:master May 20, 2015
@non non removed the in progress label May 20, 2015
lazy val state = project.dependsOn(macros, core, free, tests % "test -> test")
.settings(moduleName := "cats-state")
.settings(catsSettings)
.settings(noPublishSettings)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason not to publish this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good catch. I would be fine publishing it.

Want to submit a small PR fixing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by #312

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.

is there a place in cats for the State monad?
6 participants