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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

amesgen
Copy link

@amesgen amesgen commented Apr 25, 2022

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
Copy link
Author

amesgen commented Apr 25, 2022

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
Copy link
Contributor

treeowl commented Apr 25, 2022

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
Copy link
Contributor

treeowl commented Apr 25, 2022

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.

Copy link
Contributor

@konsumlamm konsumlamm left a comment

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
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
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
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
Copy link
Author

amesgen commented Apr 25, 2022

@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