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

Add changelog entries since v5.3.1 #209

Merged
merged 7 commits into from
Jan 21, 2021

Conversation

milesfrain
Copy link
Contributor

@milesfrain milesfrain commented Jan 18, 2021

Description of the change

This PR updates the CHANGELOG in preparation for the upcoming release of this library.

Related: purescript/purescript#3985

CHANGELOG.md Outdated
- Added monomorphic `foldl` `foldr` `foldMap` `fold` `intercalate` to `Array` (#201)
- These are just wrapped versions from `Data.Foldable`
- Added monomorphic `foldl1` `foldr1` `foldMap1` `foldl1` `intercalate` to `Array.NonEmpty` (#201)
- These are just wrapped versions from `Data.Foldable`
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure "wrapped versions" is the correct terminology here; @hdgarrood, do you have a suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think “wrapped” suggests that actually you’ve done something to them, whereas all we’ve done here is specialize. I’d suggest “specialized versions of the functions with the same names from Data.Foldable”. Is that true for all of these though? This shouldn’t be a separate bullet point either, it’s part of the same change.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, the one I’m not sure about is intercalate - I thought it was supposed to get its own Array-specific implementation.

Copy link
Member

Choose a reason for hiding this comment

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

This shouldn’t be a separate bullet point either, it’s part of the same change.

It's indented, to indicate a clarification rather than an entire new point, but I think it reads a little better as just sentences in the same bullet point and agree it should just merge into the previous line.

CHANGELOG.md Outdated
Comment on lines 13 to 15
- Added specialized monomorphic functions from `Data.Foldable` (#201):
- Added `foldl`, `foldr`, `foldMap`, `fold`, `intercalate` to `Array`
- Added `foldl1`, `foldr1`, `foldMap1`, `foldl1`, `intercalate` to `Array.NonEmpty`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reorganized the nested bullets to cut down on duplication. What's the follow-up step for intercalate mentioned in the earlier discussion?

Copy link
Contributor

Choose a reason for hiding this comment

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

The question is whether intercalate should get its own Array specific implementation (for performance) rather than just being a specialization of the intercalate from Data.Foldable. I think it probably should, or at least we should measure to see if an Array-based implementation would speed it up, but I suppose that doesn’t need to happen right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've opened #210 to track this.

CHANGELOG.md Outdated

New features:
- Added specialized monomorphic functions from `Data.Foldable` (#201):
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually find this a bit confusing, because these functions aren’t actually from Data.Foldable, they’re versions of those functions. I’d suggest “Added specialized versions of the functions from Data.Foldable”. Saying they’re specialized is the same thing as saying they’re monomorphic, and I think using both words makes this a bit harder to parse.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about

Exported monomorphic functions from Data.Foldable (i.e. f is forced to be Array)

It doesn't include the HeytingAlgebra being forced to be Boolean changes we also made (e.g. all, any), but that's the heart of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still prefer to say “Added specialized versions of the functions from Data.Foldable“. My problem is with saying the functions are from Data.Foldable (as opposed to being versions of the functions from Data.Foldable).

Copy link
Contributor

@JordanMartinez JordanMartinez left a comment

Choose a reason for hiding this comment

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

I think only one minor change needs to be made before this can be merged.

Thanks for tackling this @milesfrain!

@JordanMartinez JordanMartinez merged commit 4ec4425 into purescript:master Jan 21, 2021
@milesfrain milesfrain deleted the patch-1 branch January 21, 2021 02:12
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.

4 participants