hdgarrood / purescript-sequences

Various efficient-ish sequence types for PureScript.
http://pursuit.purescript.org/packages/purescript-sequences
MIT License
45 stars 22 forks source link

Purescript 0.9.1 #23

Closed kika closed 7 years ago

kika commented 8 years ago

This is WIP - it fails at least one test out of many successful.

I'd love to have a review of my usage of unsafePartial, not sure if convenient cowboy approach of shooting first and thinking later works here.

I skimmed through the paper, but can't say I fully understand the algorithm, that could have contributed to not very successful update. CC @bkonkle re #22

bkonkle commented 8 years ago

Some comments above were hidden by commits that made the code they commented on outdated, so you may have to expand a few comment threads above to see my questions on FingerTree, Ordered, and NonEmpty. 😀 Thanks!

texastoland commented 8 years ago

Any way I can help?

hdgarrood commented 8 years ago

If you felt like trying to pick this up again and address the remaining comments, that would certainly help :) unfortunately GitHub has hidden a few diff comment threads that still need addressing as it considers them 'outdated', just FYI.

texastoland commented 8 years ago

I noticed because Batteries needs it. No promises but I'll make an effort this week! I'll rebase first so I can see the difference from where he started in a single commit.

rgrempel commented 8 years ago

@sardonicpresence has also done some related work here:

https://github.com/sardonicpresence/purescript-sequences/commit/d385e98a415644c8ae8c365443aa41dea54b0d91

I haven't looked carefully at that, though.

texastoland commented 8 years ago

Thanks @rgrempel I tracked both branches locally.

texastoland commented 8 years ago

I'm reviewing the old branch and a lot of the Partial usages look unnecessary. Any help with https://twitter.com/texastoland/status/757448593305468928?

sardonicpresence commented 8 years ago

I made pretty heavy use of unsafePartial on my branch as there are number of guarantees made by the ADTs that are not enforced at the type-level internally. Not sure how you feel about my version of those changes specifically.

texastoland commented 8 years ago

I'll compare tonight thanks @sardonicpresence 🙏 I'm specifically struggling with viewX/deepX. I'm getting a Partial binding group error despite exhaustive cases and no explicit constraints which @garyb explained has to do with mutual recursion.

hdgarrood commented 8 years ago

I can't think of a good way to track down where Partial constraints are coming from. I guess this would be a good thing for tooling to be able to do; getting the type of a selected expression gets us most of the way there I guess, but we don't quite have that yet, do we?

texastoland commented 8 years ago

Right. As a stopgap I tried randomly injecting unsafePartial to see whether I could narrow the scope but it's happening at the top level despite my intuition that all cases are covered. I'll look afresh tonight.

garyb commented 8 years ago

There's a different error if you have a partial pattern match that is not typed with a Partial constraint - so if you're getting a warning about a missing instance, that means a function with the constraint is being used rather than a partial pattern being the problem.

texastoland commented 8 years ago

Ugh that was it. I overlooked a usage with an explicit annotation in the mutual recursion. Thanks again @garyb 🙏

adamw commented 8 years ago

Hey, I was wondering what's left to do in this PR? I suspect making sequences work with 0.9.1 is more than defining operators as aliases :)

texastoland commented 8 years ago

I've been working on it between a other PRs. The main challenge is [setting aside the time] to verify which functions really ought to be Partial. This PR assumed the majority of them which if not incorrect should at least be transformed at the type level into something more indicative of what's missing (purescript/purescript#2248). I think I need to grok the algorithm. I'll make a separate PR soon-ish if you'd like to coordinate.