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

simplify null guard #19

Merged
merged 1 commit into from
Nov 16, 2014
Merged

simplify null guard #19

merged 1 commit into from
Nov 16, 2014

Conversation

davidchambers
Copy link
Contributor

We could use !== null here instead, since typeof undefined is not 'object', but as a general rule I avoid differentiating between null and undefined.

@davidchambers
Copy link
Contributor Author

I don't know why the build failed. :\

@garyb
Copy link
Member

garyb commented Nov 16, 2014

Don't worry about it, as @paf31 mentioned in another issue we need to do something to update the travis build to use the latest psc.

@@ -25,7 +25,7 @@ class Index i where
foreign import unsafeReadPropImpl
"""
function unsafeReadPropImpl(f, s, key, value) {
if (value && typeof value === 'object') {
if (value != null && typeof value === 'object') {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we need the typeof here at all now, the if is only guarding against trying to read properties in a way that will raise runtime errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, nice! Updated to use a one-liner. :)

@davidchambers davidchambers changed the title use self-documenting null guard simplify null guard Nov 16, 2014
@garyb
Copy link
Member

garyb commented Nov 16, 2014

Ta 👍

garyb added a commit that referenced this pull request Nov 16, 2014
@garyb garyb merged commit 660a4c3 into purescript:master Nov 16, 2014
@davidchambers davidchambers deleted the guard branch November 16, 2014 01:27
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

2 participants