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

Improve isEmpty #34

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

Conversation

crgwbr
Copy link
Contributor

@crgwbr crgwbr commented Jun 22, 2021

@crgwbr crgwbr force-pushed the crweber/improve-is-empty branch 3 times, most recently from 25e76e9 to 9a1c1eb Compare June 22, 2021 18:39
@crgwbr
Copy link
Contributor Author

crgwbr commented Jun 22, 2021

Ping @yokuze

@onebytegone onebytegone requested a review from yokuze February 8, 2022 20:18
@crgwbr crgwbr force-pushed the crweber/improve-is-empty branch from 9a1c1eb to 56c9c85 Compare March 1, 2022 15:07
@onebytegone
Copy link
Contributor

@crgwbr Sorry for the delay on this PR. By any chance could you create separate PRs for a couple of the changes in this? One for the isBoolean and isSet and another for the internal change from any to unknown on the other type guards? We can likely get those smaller PRs merged within a few days, which will reduce the testing load for this PR.

@crgwbr crgwbr force-pushed the crweber/improve-is-empty branch 2 times, most recently from 9724e4d to 83932c8 Compare July 23, 2024 15:17
@crgwbr
Copy link
Contributor Author

crgwbr commented Jul 23, 2024

@onebytegone I've created #56 for the change from any to unknown and changed this PR to just by the changes to isEmpty. Couldn't quite to what you asked for though: isBoolean is required for #56, so that had to go there rather than here.

I suppose I could create 3 PRs:

  1. Add isBoolean and isSet.
  2. Use unknown types (would depend on 1).
  3. Improve isEmpty (would also depend on 1).

Let me know what you want done.

@crgwbr
Copy link
Contributor Author

crgwbr commented Jul 23, 2024

Also, as two PRs now, there's going to be a merge conflict in src/utils/is-empty.ts for whichever of these PRs gets merged second. Let me know and I'll rebase it for you.

@onebytegone
Copy link
Contributor

@crgwbr Could you split the isBoolean and isSet change into their own PR? Those can be merged with little risk and will resolve #49. The s/any/unknown/ changes will need more testing (tangentially, looking at #56, can we keep all the isEmpty changes in this PR).

@crgwbr crgwbr force-pushed the crweber/improve-is-empty branch from 83932c8 to 390571a Compare July 23, 2024 19:37
@crgwbr
Copy link
Contributor Author

crgwbr commented Jul 23, 2024

@onebytegone Here ya go:

#57

#56

This MR (#34) is based on #57 and will need rebased once that's merged.

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