Skip to content

Generalize generators from NonEmpty to Foldable1 #8

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

Merged
merged 1 commit into from
Feb 25, 2018

Conversation

matthewleon
Copy link
Contributor

This depends on merging of this PR purescript/purescript-nonempty#27

It is technically breaking, as it adds a higher bound for purescript-foldable-traversable.

@matthewleon matthewleon force-pushed the Foldable1-gens branch 4 times, most recently from e1c3cd4 to d5b4e06 Compare February 2, 2018 01:11
@matthewleon
Copy link
Contributor Author

rebased

FreqSemigroup \pos ->
case f pos of
Tuple (Just pos') _ -> g pos'
result -> result
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: we prefer not to align code with whitespace (also even nittier: too much indentation for the cases 😉)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix!

bower.json Outdated
@@ -16,17 +16,21 @@
"package.json"
],
"dependencies": {
"purescript-nonempty": "^4.0.0",
"purescript-nonempty": "matthewleon/purescript-nonempty#Foldable1",
Copy link
Member

Choose a reason for hiding this comment

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

Does this dependency need changing at all? Seems like we could remove it even, after this (although, that would make it breaking).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I guess the tests might need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, though perhaps there is a way to change that. I'll look into it. It would be a pity to make the change breaking just for tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, I guess we are breaking anyway due to the foldable-traversable bound change. That said, this dep could probably at least be moved to dev-deps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the dep is also still used in Control.Monad.Gen.Common for genNonEmpty. Now, given that NonEmpty is made an instance of Foldable1 with purescript/purescript-nonempty#27 it maybe makes sense to kill that function. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

It's not a breaking change to update bounds, unless the bound change is also breaking, so I think it's fine.

Maybe we can move genNonEmpty to NonEmpty when we next break it - we don't want to delete it, the changes here don't give us a way of generating Foldable1 :)

Copy link
Member

Choose a reason for hiding this comment

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

I merged/released -nonempty with the Foldable1 instance now too, so we can use the real dependency here too!

@matthewleon matthewleon force-pushed the Foldable1-gens branch 2 times, most recently from 646ac0f to 4aa1741 Compare February 25, 2018 17:31
@matthewleon
Copy link
Contributor Author

I've rebased on master and updated the nonempty dep to the new release.

@garyb garyb merged commit 511cce8 into purescript:master Feb 25, 2018
@garyb
Copy link
Member

garyb commented Feb 25, 2018

Thanks!

@safareli
Copy link
Contributor

FreqSemigroup and AtIndex are very clever! ✨ 💯

It makes me wish we had no limit in stack size 😝

@matthewleon
Copy link
Contributor Author

@safareli Thanks! Regarding stack size: as long as your Foldable has a tail recursive foldMap, shouldn't you be fine here?

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.

3 participants