Skip to content

Change var to const or let #608

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

Closed
wants to merge 4 commits into from
Closed

Change var to const or let #608

wants to merge 4 commits into from

Conversation

benhalverson
Copy link
Contributor

Addresses #598

Summary of changes:
This is the start of converting var's to const or let.
Looking for some initial feedback

@raphaelokon
Copy link
Contributor

LGTM. One thing worth doing would be adding http://eslint.org/docs/rules/prefer-const to the .eslintrc and rerun the the lint step. This way we would prevent accidentally adding var in future when merging PRs.

@bmuenzenmeyer
Copy link
Member

Awesome work. Since this is so significant a change I am going to point it to dev-3.0

@bmuenzenmeyer bmuenzenmeyer changed the base branch from dev to dev-3.0 February 5, 2017 13:04
@bmuenzenmeyer
Copy link
Member

@benhalverson
This is a great step in the right direction, thanks!
I merged this with dev-3.0 - all still runs. Turning on prefer-const as Raphael suggests yeilds 100+ errors. Are you willing to fix ?

@raphaelokon
Copy link
Contributor

@bmuenzenmeyer Those lint errors are just guides and does not always reflect appropriateness – just a compass. Awesome job @benhalverson

@bmuenzenmeyer
Copy link
Member

bmuenzenmeyer commented Feb 5, 2017 via email

@benhalverson
Copy link
Contributor Author

Thanks Guys. Yes I can work on the rest. I was going to ask about changing the vars in the tests also. Should that be another issue?

@bmuenzenmeyer
Copy link
Member

The tests are not linted due to their inconsistent style from years of development. I'd leave them out of scope for now and focus on the main library. If you want to do it as another issue, you are most welcome.

@benhalverson
Copy link
Contributor Author

i didn't have these conflicts on my machine. @bmuenzenmeyer I'll see if I can resolve them.

@bmuenzenmeyer
Copy link
Member

@benhalverson will fix this on my end tomorrow morning

@bmuenzenmeyer
Copy link
Member

@benhalverson I am rolling this into #610 as I fought with it enough here and there today and I think it needs a bit more oomph.

@benhalverson
Copy link
Contributor Author

Ok. Thanks

bmuenzenmeyer added a commit that referenced this pull request Feb 7, 2017
part of #610
part of the merge of #608

most unit tests working, only a few left - due to me hanging a property off the engine that @geoffp was not doing.
still need to see how it behaves during actual builds
@bmuenzenmeyer
Copy link
Member

With #610 done, I think this is in limbo. Please see my other comments.

@benhalverson
Copy link
Contributor Author

@bmuenzenmeyer do I need to do anything here?

@bmuenzenmeyer
Copy link
Member

@benhalverson mostly asking if you are okay with me closing. I had to merge it into work already apart of dev-3.0. The rub is for some reason I don't see any of your commits (meaning you won't get "credit" for the work).I think the circumstances of the merge somehow qualified all the commits as my own? This is the first time this has happened.

I will still display this PR and your name in the changelog and release notes, as you did great work!
Are you okay with that?

@benhalverson
Copy link
Contributor Author

Yep it's ok with me

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