-
Notifications
You must be signed in to change notification settings - Fork 8
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
Conversation
e1c3cd4
to
d5b4e06
Compare
rebased |
src/Control/Monad/Gen.purs
Outdated
FreqSemigroup \pos -> | ||
case f pos of | ||
Tuple (Just pos') _ -> g pos' | ||
result -> result |
There was a problem hiding this comment.
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 😉)
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
:)
There was a problem hiding this comment.
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!
646ac0f
to
4aa1741
Compare
4aa1741
to
e4ad3e8
Compare
I've rebased on |
Thanks! |
It makes me wish we had no limit in stack size 😝 |
@safareli Thanks! Regarding stack size: as long as your |
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
.