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

Update "Comparison Operators & Equality" in README.md #674

Merged
merged 1 commit into from
Jan 14, 2016

Conversation

pgeiss
Copy link
Contributor

@pgeiss pgeiss commented Jan 12, 2016

The guide should be more clear that [] is truthy because arrays are objects. As is, a reader could be confused about this because an empty string will be evaluated as false and it's possible (but not a good idea) to access a string using array-like syntax. It also doesn't help that this array-like syntax is in line with other languages like C where strings ARE arrays, further adding to potential confusion.

@ljharb
Copy link
Collaborator

ljharb commented Jan 12, 2016

[] is truthy, it doesn't === true.

The only 'falsy' values in JavaScript are undefined, null, 0, -0, NaN, '', and false. The 'truthy' values are all other values. All objects are truthy. There is nothing both truthy and falsy, or neither truthy nor falsy.

It might be good to indicate this somewhere, but I do think that a basic level of understanding of JS is OK to assume.

@pgeiss
Copy link
Contributor Author

pgeiss commented Jan 12, 2016

I suppose I should have used '==' in my request then. In any event, I didn't write '===' anywhere in the file (commit title, perhaps... but I can't really fix that without committing again. If you'd like me to, I can), so I'll just edit the request.

@ljharb
Copy link
Collaborator

ljharb commented Jan 12, 2016

[] == true is false also - it's !![] that's true.

@pgeiss
Copy link
Contributor Author

pgeiss commented Jan 12, 2016

Really now. Wow, I assume way too much about the way JS logic works. Perhaps I should just write that it will evaluate as true (since that's what will happen in the end if you let the interpreter decide)?

@ljharb
Copy link
Collaborator

ljharb commented Jan 12, 2016

The most precise way to describe it is with "truthy" and "falsy" imo.

@pgeiss
Copy link
Contributor Author

pgeiss commented Jan 12, 2016

Does that look better?
ps, sorry for the trouble. I've never actually tried to make a PR before so if I did this wrong it's probably my fault.

@ljharb
Copy link
Collaborator

ljharb commented Jan 13, 2016

Sure, this looks great! Are you perhaps able to rebase this down to a single commit?

@pgeiss
Copy link
Contributor Author

pgeiss commented Jan 13, 2016

I think the rebase worked (it created 5820bcd which contains both commits with the proper title), but that empty commit c2ea6c2 snuck its way in when I did git push. Nothing I do locally can touch it even after pull, re-checkout, and even using its hash in the rebase program instead of HEAD~#. git log knows it exists, but git rebase seems to pretend it doesn't. Please tell me if it's a problem.

@ljharb
Copy link
Collaborator

ljharb commented Jan 13, 2016

@pgeiss try git rebase -i HEAD~4 and in the resulting vim prompt, change the last 3 "pick"s to "s" (for squash), and then :wq - and in the resulting vim prompt, edit the commit message. then force push.

@pgeiss
Copy link
Contributor Author

pgeiss commented Jan 13, 2016

Long post about issues caused by forgetting a --force on git push redacted because it was very long

@ljharb
Copy link
Collaborator

ljharb commented Jan 13, 2016

@pgeiss the easiest is to make a brand new branch, and force push to your patch-1 branch - which will update this PR in-place. You definitely have to force push whenever you rebase.

@pgeiss
Copy link
Contributor Author

pgeiss commented Jan 13, 2016

Got it! I guess all my issues were because I wasn't force pushing. Anyways, should be all set now.

@pgeiss
Copy link
Contributor Author

pgeiss commented Jan 14, 2016

Rebased again after #675-#677 (677 didn't change README.md).

@ljharb ljharb merged commit 9498b3f into airbnb:master Jan 14, 2016
@ljharb
Copy link
Collaborator

ljharb commented Jan 14, 2016

Sure, but this way I don't have to have a merge commit :-) Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants