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

Idiomatic Show instance for lazy lists #181

Merged

Conversation

kl0tl
Copy link
Member

@kl0tl kl0tl commented Dec 20, 2020

This pull request changes the output of show for Data.List.Lazy.Types.List and Data.List.Lazy.Types.NonEmptyList from this:

> Data.List.Lazy.fromFoldable [0, 1]
fromStrict ((Cons 0 (Cons 1 Nil)))

> Data.List.Lazy.NonEmpty.fromFoldable [0, 1]
(Just (NonEmptyList (defer \_ -> (NonEmpty 0 fromStrict ((Cons 1 Nil))))))

to that:

> Data.List.Lazy.fromFoldable [0, 1]
(defer \_ -> 0 : (defer \_ -> 1 : (defer \_ -> Nil)))

> Data.List.Lazy.NonEmpty.fromFoldable [0, 1]
(Just (NonEmptyList (defer \_ -> (NonEmpty 0 (defer \_ -> 1 : (defer \_ -> Nil))))))

Alternatively, we could show (fromFoldable […]) but this makes the output of show less similar:

> Data.List.Lazy.fromFoldable [0, 1]
(fromFoldable [1,0])

> Data.List.Lazy.NonEmpty.fromFoldable [0, 1]
(Just (NonEmptyList (defer \_ -> (NonEmpty 0 (fromFoldable [1])))))

Close #122.

@natefaubion
Copy link
Contributor

While this is technically code that does indeed work, I'm not sure how useful it is as a Show instance since it's very much not compact. It's very difficult to see what the values are in the midst of all the other noise (and parens, which will quickly get out of hand). I feel like it kind of misses the forest for the trees as far as helping someone see what's actually in the list.

@milesfrain
Copy link
Contributor

I feel like it kind of misses the forest for the trees as far as helping someone see what's actually in the list.

Seems like this would be less of a dilemma if we adopted debugged.

@@ -34,6 +34,10 @@ newtype List a = List (Lazy (Step a))
-- | `Cons` constructor).
data Step a = Nil | Cons a (List a)

instance showStep :: Show a => Show (Step a) where
show Nil = "Nil"
show (Cons x xs) = show x <> " : " <> show xs
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this not be wrapped in parens?

@natefaubion
Copy link
Contributor

I personally think fromFoldable is the way to go for this.

@hdgarrood
Copy link
Contributor

I still want to adopt debugged in core, but in any case I think we should continue providing Show instances for the foreseeable future.

@JordanMartinez
Copy link
Contributor

Let's use fromFoldable. Who's going to do the work?

@kl0tl
Copy link
Member Author

kl0tl commented Jan 9, 2021

I'll come back to this once I'm done with the compiler releases notes. I'll never get them done if I find other ways to spend m'y PureScript budget 😬

@JordanMartinez
Copy link
Contributor

Does this PR only need these two changes?

-- adding parenthesis around `Step`'s `Cons` case
instance showStep :: Show a => Show (Step a) where
  show Nil = "Nil"
  show (Cons x xs) = "(" <> show x <> " : " <> show xs <> ")"

-- using the 'fromFoldable' approach:
instance showList :: Show a => Show (List a) where
  show ls = "fromFoldable [" <> print (step ls) <> "]"
    where
    print Nil = ""
    print (Cons x xs') = show x <> go (step xs')

    go Nil = ""
    go (Cons x xs') = ", " <> show x

@JordanMartinez
Copy link
Contributor

@kl0tl Mind if I push the changes I mentioned above here? If that's all we need to do, then we can get this PR merged.

@kl0tl
Copy link
Member Author

kl0tl commented Jan 30, 2021

I pushed a patch based on your suggestion! Here’s the output of show for Data.List.Lazy.Types.List, Data.List.Lazy.Types.Step and Data.List.Lazy.Types.NonEmptyList:

> Data.List.Lazy.fromFoldable [0, 1, 2]
(fromFoldable [0,1,2])

> Data.List.Lazy.NonEmpty.fromFoldable [0, 1, 2]
(Just (NonEmptyList (defer \_ -> (NonEmpty 0 (fromFoldable [1,2])))))

> Data.List.Lazy.step (Data.List.Lazy.fromFoldable [0, 1, 2])
(0 : (fromFoldable [1,2]))

@kl0tl kl0tl force-pushed the idiomatic-show-instance-for-lazy-lists branch from 91f5f05 to acfae4f Compare January 30, 2021 16:09
@kl0tl
Copy link
Member Author

kl0tl commented Jan 30, 2021

I also opened purescript/purescript-ordered-collections#46 for consistency, so that we display arrays instead of lists when showing (non empty) sets.

@JordanMartinez
Copy link
Contributor

Any other feedback before we merge this?

@JordanMartinez JordanMartinez merged commit a949279 into purescript:master Feb 3, 2021
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.

Data.List.Lazy: strange Show instance
6 participants