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

Simplify implementation of internal FFI function intercalate in Data.Show.Generic #274

Merged
merged 4 commits into from
Mar 16, 2022

Conversation

cdepillabout
Copy link
Contributor

@cdepillabout cdepillabout commented Sep 26, 2021

Description of the change

The Data.Show and Data.Show.Generic modules both use an internal FFI helper function intercalate. It was called join in Data.Show.

This PR does the following:

  • Simplifies the implementation in Data.Show.Generic.intercalate. This now matches the same (simple) implementation from Data.Show.
  • Renames the function in Data.Show to be intercalate. My reasoning is that the function should be named the same thing in both Data.Show and Data.Show.Generic. I thought intercalate would be easier to understand for a PureScriter, since intercalate is already used in modules like Data.Foldable. (Alternatively, intercalate could be renamed join in both modules, since join is the name of the underlying FFI function. I decided against this since join has a different meaning in PureScript. I thought intercalate was easier to understand.)

Checklist:

  • Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

@cdepillabout
Copy link
Contributor Author

I didn't add a changelog entry for this, since this change shouldn't be visible to any users.

@cdepillabout cdepillabout changed the title Intercalate Simplify implementation of internal FFI function intercalate in Data.Show.Generic Sep 26, 2021
@thomashoneyman
Copy link
Member

@cdepillabout This can be added to the CHANGELOG under the "Other improvements" section if you would like, but it isn't necessary if you don't want to.

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.

Perhaps a changelog should be added for other backend maintainers because the function name is changing from join to intercalate?

@cdepillabout
Copy link
Contributor Author

Thanks for the review.

I've added a CHANGELOG entry for this change. Hopefully this should be good to merge now.

@JordanMartinez
Copy link
Contributor

I'd like to merge this, but I'll clarify how this change affects non-JS backends before doing so. While this isn't a breaking change for the JS backend, I wonder if it is for non-JS backends.

@JordanMartinez JordanMartinez added the type: breaking change A change that requires a major version bump. label Sep 28, 2021
@JordanMartinez
Copy link
Contributor

Per Nate, this counts as a breaking change for non-JS backends. So, we shouldn't merge it until we're making breaking changes in general.

I've added that label here.

@JordanMartinez
Copy link
Contributor

🏓 @thomashoneyman

@JordanMartinez JordanMartinez merged commit a273e03 into purescript:master Mar 16, 2022
@cdepillabout cdepillabout deleted the intercalate branch March 17, 2022 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: breaking change A change that requires a major version bump.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants