Skip to content

Fix broken example, severity of warning #1570

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

Merged
merged 1 commit into from
Sep 27, 2017
Merged

Fix broken example, severity of warning #1570

merged 1 commit into from
Sep 27, 2017

Conversation

thepeoplesbourgeois
Copy link
Contributor

The problem with linting rules sometimes is that they can imply that not having a semicolon somewhere in thousands of lines of code (that runs perfectly fine without them) is equally as bad as accidentally writing a loop that crashes your application.

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

It is indeed as bad; things that crash your app are actually less bad then things that might silently do the wrong thing, like omitting a semicolon.

@@ -381,10 +381,10 @@ Other Style Guides

```javascript
// bad
const bar = [...foo].map(bar);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do these need to change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're declaring bar. If you're declaring it here, how can it be what you pass in to map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this line of code works as it's written, I am more terrified of JavaScript than ever before

Copy link
Collaborator

Choose a reason for hiding this comment

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

aha, true :-)

@thepeoplesbourgeois
Copy link
Contributor Author

And what misbehavior arises from a missing semicolon, exactly?

@thepeoplesbourgeois
Copy link
Contributor Author

thepeoplesbourgeois commented Sep 27, 2017

I keep hearing nebulous fears from semicolon stalwarts that "bad things could happen", but I've yet to hear of any examples of such things. If they exist, surely, it should be easy enough to convince me of the character's utility

@ljharb
Copy link
Collaborator

ljharb commented Sep 27, 2017

I don't really have time to go into all the ways that ASI can and has caused numerous production bugs, but I think it's a bit presumptive to believe that something the majority of JS devs avoid isn't actually bad :-)

README.md Outdated
@@ -400,18 +400,18 @@ Other Style Guides
// good
[1, 2, 3].map(x => x + 1);

// bad
// very bad - error bad - no returned value means `memo` becomes undefined after the first iteration
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think just "bad" here, with your english explanation, is sufficient

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair. I've taken it back down just to "bad"

@thepeoplesbourgeois
Copy link
Contributor Author

I believe the logical fallacy you've just hit upon is "popularity confirmation bias", no?

I don't need "all the ways". If you can just tell the last time it's been a gotcha for you, I will concede the point.

@ljharb
Copy link
Collaborator

ljharb commented Sep 27, 2017

@thepeoplesbourgeois no, that would be if i claimed that it was true because lots of people do it. I'm claiming that someone who's new and/or uninformed should assume by default that if it's popular it's true, instead of assuming by default that it's some kind of unsubstantiated groupthink.

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks! Would you mind rebasing this down to a single commit, and prefixing the message with [guide] ? If not I can take care of it.

@thepeoplesbourgeois
Copy link
Contributor Author

Well... how new do you believe I am to Javascript? :)

@ljharb
Copy link
Collaborator

ljharb commented Sep 27, 2017

lol, fair point, i apologize that i took your questions as implied newness.

@thepeoplesbourgeois
Copy link
Contributor Author

It happens 😁 I'll make the updates to the branch, but it'll be in a little while. I've been doing these edits in the online editor, so I need to find a moment to clone the repo and perform the terminal commands. Thank you for being my first approved open-source contrib!

@ljharb
Copy link
Collaborator

ljharb commented Sep 27, 2017

oh no worries then; i'll take care of the rebase for you :-) Thanks for the contribution!

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