-
Notifications
You must be signed in to change notification settings - Fork 947
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
Replace var in javascript with let/const #5558
Conversation
Generated by 🚫 Danger |
Not in unmerged pull requests. |
728d991
to
4e28e7f
Compare
If this is merged, I'll have to rewrite #5064 again. That's a bugfix PR. People are still complaining about bugs it fixes. |
@AntonKhorev I'd still say merge sooner rather than later just because all the arising issues can be autofixed. |
4e28e7f
to
f3e092f
Compare
99fb2a9
to
f6416cd
Compare
f6416cd
to
0d31f55
Compare
app/assets/javascripts/index.js
Outdated
url: content_path, | ||
url: path, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're already cleaning up after me, you can define the function as function (url, callback)
for even more conciseness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe not given #5663
0d31f55
to
32c1df3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All occurrences of var
are changed with let/const
. Checked all changed variables and let
and const
are set accordingly (every variable that is not changed after declaration, is const
). Also spent some time on website to test it generally, if any page threw error in the console. Everything I checked works as expected, there are no errors.
ESLint concurs (after 1.88 seconds) |
83dde9e
to
61760af
Compare
Can we merge this please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs rebase, but otherwise looks good to me. I agree it would be good to merge this as soon as we can.
61760af
to
a2de840
Compare
I've rebased this now. |
a2de840
to
c47cb29
Compare
Merged, thanks! |
This updates the eslint config to prohibit use of
var
and to preferconst
overlet
and then fixes all uses ofvar
to respect that.