Skip to content

Add isEmpty #222

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

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

Conversation

sigma-andex
Copy link

@sigma-andex sigma-andex commented Jun 27, 2022

Description of the change

Adds a human-friendly version of null: isEmpty


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)

@sigma-andex
Copy link
Author

🐧 @JordanMartinez

@sigma-andex
Copy link
Author

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

@JordanMartinez
Copy link
Contributor

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

@sigma-andex
Copy link
Author

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

@natefaubion
Copy link

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

@JordanMartinez
Copy link
Contributor

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.

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.

@sigma-andex Can you add the deprecation notice to null and update the changelog to mention that?

@natefaubion
Copy link

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

@sigma-andex
Copy link
Author

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

@JordanMartinez
Copy link
Contributor

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?

@natefaubion
Copy link

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
Copy link

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.

@sigma-andex
Copy link
Author

sigma-andex commented Jun 28, 2022

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

@natefaubion
Copy link

natefaubion commented Jun 28, 2022

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.

@JordanMartinez
Copy link
Contributor

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

@sigma-andex
Copy link
Author

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

@JordanMartinez
Copy link
Contributor

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.

@sigma-andex
Copy link
Author

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

@JordanMartinez
Copy link
Contributor

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.

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.

3 participants