purescript-arrays
purescript-arrays copied to clipboard
Add isEmpty
Description of the change
Adds a human-friendly version of null
: isEmpty
Checklist:
- [x] 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
- [x] Updated or added relevant documentation
- [ ] Added a test for the contribution (if applicable)
🐧 @JordanMartinez
PD: The huge diff comes from purs-tidy. Seems it hasn't been formatted before. Can undo if you prefer a smaller diff.
Formatting via purs tidy was something we were going to do as a series of maintenance PRs after 0.15
was released. That should likely be its own PR.
As for this PR, ugh. :smile: I've never really liked using Array.null
to test whether an array is empty because null
doesn't mean empty
to me. However, adding an alias for the same function feels wrong to me, too. Why have two functions that do the same thing? And yet, renaming null
to isEmpty
also feels like it somehow goes against the Haskell tradition (besides being a breaking change).
Formatting via purs tidy was something we were going to do as a series of maintenance PRs after
0.15
was released. That should likely be its own PR.
Ok can undo the formatting.
because
null
doesn't meanempty
to me.
yes it's the same for me and also for others. isEmpty
is just the more meaningful name and it is also used already in other data types (Map
, Set
, ForeignObject
). I think sth like isEmpty
is also what a newcomer would expect rather than null
. I also would like to add it to string because String.null s
is just meh
Regarding the alias: I think this is the best compromise to not have a breaking change and not repeat the code
I'm personally in favor of using isEmpty
everywhere, and adding deprecated constraints to null
. null
is a terrible name.
And yet, renaming null to isEmpty also feels like it somehow goes against the Haskell tradition (besides being a breaking change).
To clarify my original comment, I wasn't sure if this was one of those "that ship has sailed" issues or not.
To be clear, I’m not totally sure we should merge this now vs the more normal breaking release cycle.
To be clear, I’m not totally sure we should merge this now vs the more normal breaking release cycle.
Is it breaking with Warning? Only if one uses psa right? Alternatively we can add it and then add the Warning constraint in the next release
To be clear, I’m not totally sure we should merge this now vs the more normal breaking release cycle.
Why not? This change isn't a breaking change and it makes it easier for us to know to remove null
when we do make breaking changes by the fact that it has a deprecation notice.
Were you thinking there wouldn't be a deprecation notice at all, and we would just rename null
to isEmpty
in the next breaking change?
As a core lib, and a change to a frequently used API, I think it should have a long deprecation cycle. Having experience with MonadZero warnings, they are extremely irritating. I don’t think we should add a deprecation warning until we are ready for everyone to migrate. Please don’t be hasty. There’s absolutely no reason to be.
It would be helpful to document all libraries in core that use null
(I think there's still List?). And I think we should at minimum solicit community feedback via discourse.
Ok I removed the deprecation warning again.
It would be helpful to document all libraries in core that use
null
(I think there's still List?).
I can have a look at this.
Besides nub
(see #223) there are probable some other horrible names that could be improved. I'm thinking of e.g. cons
/snoc
(could be prepend
/append
).
append
We already have append
in Semigroupoid, and it isn't the same thing. I do not think it's worth changing at that fundamental of a level. I'll note that I think individual API names should all have their own proposals and community feedback. These are supposed to be stable APIs and there is necessarily going to be a lot of bikeshedding.
@sigma-andex Can you compile a list of all APIs you think should be renamed and open a Discourse post for each one? For example,
- 1 Discourse post for renaming
null
toisEmpty
across multiple libraries (e.g.arrays
,lists
,strings
) - 1 Discourse post for renaming
snoc
/cons
to ??? - 1 Discourse post for renaming
nub
to ???
I think a Discourse post following this general template would work:
Title:
Proposal: renaming `functionName
Body:
List all the libraries with a function by that name and what the function does. Propose a new name for the function and list reasons for why. List other potential new names for the function. List potential name clashes for each new name and demo what a qualified usage (e.g.
Array.isEmpty
) would look like.
For example, something like this:
Proposal: renaming the null
function
null
is a function used in arrays
, lists
, and strings
to determine whether the corresponding value is empty. I propose we rename the null
function to isEmpty
for the following reasons:
-
isEmpty
is more readable thannull
in that it actually describes what the function does so that even newcomers know it - Haskell uses
null
(and hence why we do, too), and I would argue that we should not follow Haskell here.
Other potential new names: none that come to mind
Potential name clashes with other pre-existing names:
- Map.isEmpty
- Set.isEmpty
To workaround these name clashes, one would need to use a qualified import (e.g. Array.isEmpty
)
@JordanMartinez I can have a look at it. Discord has a poll feature, so we could just open (multiple choice) polls for each option. Don't you think it would be better to just have one discord post?
Don't you think it would be better to just have one discord post?
No because I expect there to be a lot of bikeshedding. In some posts (e.g. isEmpty
), I doubt many will be against the change. In others (e.g. nub
), there could be wide disagreement. I'd like to keep such posts focused on one particular change so that others can get through faster.
What should be the deadline for the pools? Like 4 weeks?
4 weeks sounds good to me. That being said, I don't think the polls will be the only factor when considering what to do here. In other words, if there is a majority that support some new name foo
, we may still rename it to bar
. Even though people will vote in the poll, it doesn't mean everyone who had an interest in the discussion knew about it or made a vote.