Skip to content

Fix mention of foldl' in Haddocks of unions/unionsWith#832

Open
amesgen wants to merge 1 commit into
haskell:masterfrom
amesgen:unions-foldl'-docs
Open

Fix mention of foldl' in Haddocks of unions/unionsWith#832
amesgen wants to merge 1 commit into
haskell:masterfrom
amesgen:unions-foldl'-docs

Conversation

@amesgen

@amesgen amesgen commented Apr 25, 2022

Copy link
Copy Markdown

Tiny documentation fix, was confused by this for a split second today 😆

Before:
2022-04-25_21-50
After:
2022-04-25_21-50_1

@amesgen

amesgen commented Apr 25, 2022

Copy link
Copy Markdown
Author

Another option would be to remove the inlined implementation from the Haddocks all-together, as the up-to-date actual implemenation is just one click away.

@treeowl

treeowl commented Apr 25, 2022

Copy link
Copy Markdown
Contributor

The documentation should have everything a user needs to know to understand both the meaning of the function and its performance. In this case, I think the easiest way to do that is to give the actual implementation.

@treeowl

treeowl commented Apr 25, 2022

Copy link
Copy Markdown
Contributor

In the future, please refrain from including images in issues or issue discussions unless they contribute substantially; they are generally bad for accessibility. A diagram of an idea or a rendering of a formatting change would be fine, but these images weren't helpful.

@konsumlamm konsumlamm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's actually another mistake: The functions work on any Foldable, not just lists.

@@ -1773,7 +1773,7 @@ maxView t = case maxViewWithKey t of
Union.
--------------------------------------------------------------------}
-- | The union of a list of maps:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
-- | The union of a list of maps:
-- | The union of a 'Foldable' of maps:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would go with "'Foldable' container".

@@ -1788,7 +1788,7 @@ unions ts
#endif

-- | The union of a list of maps, with a combining operation:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
-- | The union of a list of maps, with a combining operation:
-- | The union of a 'Foldable' of maps, with a combining operation:

@amesgen

amesgen commented Apr 25, 2022

Copy link
Copy Markdown
Author

@konsumlamm Personally, I have no opinion on whether that is misleading (after all, Foldable can be characterized by toList). In any case, if this change is desired, it should also be changed in Data.Map.Strict.Internal/ Data.Set.Internal/Data.IntSet.Internal, which might or might not be in scope of this PR.

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