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

generalizes deleteBy parameter type #245

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

acple
Copy link

@acple acple commented Feb 14, 2024

This generalizes deleteBy parameter without any implementation changes.

Current problem is that we must have actual value or a dummy of the type of array item while using deleteBy:

type Id = Int
type ComplicatedRecord = { id :: Id, a :: String, b :: Boolean, c :: ... }

deleteById :: Id -> Array ComplicatedRecord -> Array ComplicatedRecord
deleteById id = ? -- cannot implement with `deleteBy` unless creating dummy value of ComplicatedRecord

This PR allows it like this:

deleteById :: Id -> Array ComplicatedRecord -> Array ComplicatedRecord
deleteById = deleteBy \id item -> id == item.id

I believe this PR is almost non-breaking, however, there is a slight possibility of breakage related to type inference things.
I verified that the all of latest package-set items can be built with this change.


Alternative approach:

delete :: forall a. Eq a => a -> Array a -> Array a
delete x = deleteBy (eq x)

deleteBy :: forall a. (a -> Boolean) -> Array a -> Array a
deleteBy p xs = _

-- usage
deleteById :: Id -> Array _ -> Array _
deleteById id = deleteBy \item -> id == item.id

This has more clear signature similar to filter and easy to understand but is a Breaking-change.


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)

@acple
Copy link
Author

acple commented Feb 15, 2024

I decided put this change as Other improvements rather than Breaking changes or New features.
I'm not sure which category was most appropriate for this.

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.

2 participants