Skip to content

Conversation

@alanshaw
Copy link
Member

  • Throws if addr is falsy value such as 0, false or NaN
  • Allows if null, undefined, ''

Copy link
Member

@vmx vmx left a comment

Choose a reason for hiding this comment

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

Could you please add a test case for undefined?

src/index.js Outdated

// default
addr = addr || ''
addr = addr == null ? '' : addr
Copy link
Member

Choose a reason for hiding this comment

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

What about:

if (addr == null) {
  addr = ''
}

That seems easier to read for future generations ;)

Copy link
Member

Choose a reason for hiding this comment

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

It took my a while to figure out that == null also matches undefined, I'm just so used to ===. I personally would prefer a if (addr === undefined || addr === null). But that's surely a matter of style/taste, so feel free to leave it with == null.

@alanshaw
Copy link
Member Author

@vmx is good?

Copy link
Member

@vmx vmx left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fixes!

@vmx vmx merged commit 1a81317 into master Feb 27, 2018
@daviddias daviddias deleted the fix/tighten-addr-validation branch February 27, 2018 16:28
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