This repository has been archived by the owner. It is now read-only.

requireVarDeclFirst vs const/let #1783

Closed
markelog opened this Issue Sep 13, 2015 · 12 comments

Comments

Projects
None yet
6 participants
@markelog
Member

markelog commented Sep 13, 2015

Those are block scoped, so it doesn't makes sense to require them to be the first statements in function.

So we need to either apply more comprehensive algorithm or remove them.

/cc @oredi

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 14, 2015

Contributor

@markelog when requireVarDeclFirst was implemented I did not consider ES6 at that time. I think I could look into allowing let to have block scope. This might take sometime though, is it a time critical requirement or can we let #1069 get released without the requireVarDeclFirst rule and add that to the preset once I can swing around to fix it. I am looking into a 3-4 weeks time frame as the past 2 weeks will be tight for me to look into this. :)

Contributor

ghost commented Sep 14, 2015

@markelog when requireVarDeclFirst was implemented I did not consider ES6 at that time. I think I could look into allowing let to have block scope. This might take sometime though, is it a time critical requirement or can we let #1069 get released without the requireVarDeclFirst rule and add that to the preset once I can swing around to fix it. I am looking into a 3-4 weeks time frame as the past 2 weeks will be tight for me to look into this. :)

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Sep 14, 2015

Member

For sure! No hurry ;)

Member

markelog commented Sep 14, 2015

For sure! No hurry ;)

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 14, 2015

Contributor

I will look into that when my schedule frees up.
Something out of topic but I love the new reporting for npm test

image

Contributor

ghost commented Sep 14, 2015

I will look into that when my schedule frees up.
Something out of topic but I love the new reporting for npm test

image

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Sep 21, 2015

Member

This should be added to the idiomatic preset.

Member

hzoo commented Sep 21, 2015

This should be added to the idiomatic preset.

@dsebastien

This comment has been minimized.

Show comment
Hide comment
@dsebastien

dsebastien Sep 23, 2015

+1 for a change of behavior of this property. I disabled it for now, way too annoying w/ ES2015 :)

dsebastien commented Sep 23, 2015

+1 for a change of behavior of this property. I disabled it for now, way too annoying w/ ES2015 :)

@lukebennett

This comment has been minimized.

Show comment
Hide comment
@lukebennett

lukebennett Sep 29, 2015

Yep, agree with the need to change this rule.

lukebennett commented Sep 29, 2015

Yep, agree with the need to change this rule.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 2, 2015

Contributor

I might have sometime next week, I will try looking into this. :)

Contributor

ghost commented Oct 2, 2015

I might have sometime next week, I will try looking into this. :)

@bturk

This comment has been minimized.

Show comment
Hide comment
@bturk

bturk Oct 17, 2015

Another +1.

When you implement don't forget you can't just look for braces:

for (let item in obj) and for (let i = 0; i < len; i++) are implicitly block scoped.

bturk commented Oct 17, 2015

Another +1.

When you implement don't forget you can't just look for braces:

for (let item in obj) and for (let i = 0; i < len; i++) are implicitly block scoped.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 17, 2015

Contributor

Sure. Sorry I had to delay on this as I just let my company and had a little cleaning up to do on the day job.

Contributor

ghost commented Oct 17, 2015

Sure. Sorry I had to delay on this as I just let my company and had a little cleaning up to do on the day job.

@bturk

This comment has been minimized.

Show comment
Hide comment
@bturk

bturk Oct 17, 2015

No big rush, I've been doing JavaScript for a long time so it's not very natural to me to put variables anywhere else, let will take some getting used to.

Actually, now that I've thought about it, I'm surprised this rule gets triggered at all. Seems it would specifically be looking for var. Just a suggestion, it's your product and it's a good one but I'd split the rule.

requireVarDeclFirst - only look at var
requireLetDeclFirst - only look at let/const and at block level

bturk commented Oct 17, 2015

No big rush, I've been doing JavaScript for a long time so it's not very natural to me to put variables anywhere else, let will take some getting used to.

Actually, now that I've thought about it, I'm surprised this rule gets triggered at all. Seems it would specifically be looking for var. Just a suggestion, it's your product and it's a good one but I'd split the rule.

requireVarDeclFirst - only look at var
requireLetDeclFirst - only look at let/const and at block level

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Oct 17, 2015

Member

If people want it, I can create a change now that ignores let/const and release another patch version

Member

hzoo commented Oct 17, 2015

If people want it, I can create a change now that ignores let/const and release another patch version

@hzoo hzoo added this to the 2.4.0 milestone Oct 17, 2015

@bturk

This comment has been minimized.

Show comment
Hide comment
@bturk

bturk Oct 17, 2015

That would work for me. I could remove the override comments and add the new rule later if you do it that way.

bturk commented Oct 17, 2015

That would work for me. I could remove the override comments and add the new rule later if you do it that way.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.