purescript-arrays icon indicating copy to clipboard operation
purescript-arrays copied to clipboard

Add isEmpty

Open sigma-andex opened this issue 2 years ago • 18 comments

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)

sigma-andex avatar Jun 27 '22 09:06 sigma-andex

🐧 @JordanMartinez

sigma-andex avatar Jun 27 '22 09:06 sigma-andex

PD: The huge diff comes from purs-tidy. Seems it hasn't been formatted before. Can undo if you prefer a smaller diff.

sigma-andex avatar Jun 27 '22 09:06 sigma-andex

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).

JordanMartinez avatar Jun 27 '22 12:06 JordanMartinez

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 mean empty 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

sigma-andex avatar Jun 27 '22 13:06 sigma-andex

I'm personally in favor of using isEmpty everywhere, and adding deprecated constraints to null. null is a terrible name.

natefaubion avatar Jun 27 '22 17:06 natefaubion

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.

JordanMartinez avatar Jun 28 '22 01:06 JordanMartinez

To be clear, I’m not totally sure we should merge this now vs the more normal breaking release cycle.

natefaubion avatar Jun 28 '22 13:06 natefaubion

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

sigma-andex avatar Jun 28 '22 13:06 sigma-andex

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?

JordanMartinez avatar Jun 28 '22 13:06 JordanMartinez

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.

natefaubion avatar Jun 28 '22 14:06 natefaubion

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.

natefaubion avatar Jun 28 '22 14:06 natefaubion

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).

sigma-andex avatar Jun 28 '22 15:06 sigma-andex

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.

natefaubion avatar Jun 28 '22 16:06 natefaubion

@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 to isEmpty 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:

  1. isEmpty is more readable than null in that it actually describes what the function does so that even newcomers know it
  2. 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 avatar Jun 29 '22 13:06 JordanMartinez

@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?

sigma-andex avatar Jun 29 '22 13:06 sigma-andex

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.

JordanMartinez avatar Jun 29 '22 14:06 JordanMartinez

What should be the deadline for the pools? Like 4 weeks?

sigma-andex avatar Jun 29 '22 14:06 sigma-andex

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.

JordanMartinez avatar Jun 29 '22 17:06 JordanMartinez