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

Add is.empty() #16

Merged
merged 2 commits into from
Oct 6, 2017
Merged

Add is.empty() #16

merged 2 commits into from
Oct 6, 2017

Conversation

kodie
Copy link
Contributor

@kodie kodie commented Oct 5, 2017

Added the is.empty(value) function as mentioned in #1.

index.js Outdated
@@ -163,4 +163,12 @@ is.inRange = (x, range) => {

is.infinite = x => x === Infinity || x === -Infinity;

is.empty = value => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you've got quite a lot going on in this method.

  1. get rid of unnecessary wrapping ( ).
  2. value.length === 0 over !value.length
  3. you basically have 3 assertions inside (aka assertion1 || assertion2 || assertion3). extract each assertion to a helper method (like hasPromiseAPI), and invoke them inside is.empty. This will also help identify which case each assertion relates to, and that will also help clarify which part of the method each assertion in the tests relates to.
  4. what is the outcome of is.empty(undefined) or is.empty(null)? we need to make sure that all cases are taken care of, as the README states,

readme.md Outdated
@@ -150,6 +150,11 @@ is.inRange(3, 10);
Check if `value` is `Infinity` or `-Infinity`.


##### .empty(value)

Returns `true` if `value` is an empty string, array, object, map, or set.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rephrase it to match all other methods:

Check if value is an empty string, array, object, map or set

@@ -373,3 +373,26 @@ test('is.inRange', t => {
test('is.infinite', t => {
testType(t, 'infinite', ['number']);
});

test('is.empty', t => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add cases for null and undefined.

@gioragutt
Copy link
Collaborator

@kodie nice PR, personally been waiting for this in my app. Couple of changes for readability and clean code, but it seems fine otherwise.

@kodie
Copy link
Contributor Author

kodie commented Oct 5, 2017

@gioragutt Fixing those right now!

Quick question, what should booleans return?

is.empty(true);
is.empty(false);

?

@gioragutt
Copy link
Collaborator

@kodie not quite sure, I believe that is.empty(boolean) === !boolean, but that's a thought.

@sindresorhus your thought?

@kodie
Copy link
Contributor Author

kodie commented Oct 5, 2017

@gioragutt I've made the requested changes. I made it just return true if given a falsy value. So true will return false but false, null, and undefined will all return true.

Copy link
Collaborator

@gioragutt gioragutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good changes. Waiting for @sindresorhus to review

@gioragutt gioragutt mentioned this pull request Oct 5, 2017
Closed
9 tasks
@sindresorhus sindresorhus merged commit 46e886d into sindresorhus:master Oct 6, 2017
@sindresorhus
Copy link
Owner

Looks good. Thank you @kodie. Keep 'em coming 👌😎

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.

None yet

3 participants