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

Fix#261 Replace vars with lets #263

Merged
merged 5 commits into from Oct 8, 2019

Conversation

@haichuan0424
Copy link

haichuan0424 commented Oct 7, 2019

All vars in price.js can be replaced with lets since they are used within the declaration scope. However, some of the variable names are exact the same for function parameters and local variables, which might cause confusion. You might want to file an issue to rename some of them.

For file purge.js, I moved variable "newamount" declaration outside the if/else block so it can be declared with let.

In conclusion, the fix replaced all vars with lets. Only two files price.js and purge.js are involved. There is no "var" left in any file. Let me know if any issues or further changes needed.

Copy link
Member

nikooo777 left a comment

LGTM!

@nikooo777

This comment has been minimized.

Copy link
Member

nikooo777 commented Oct 8, 2019

Thank you, this looks good to me. I haven't checked the codebase for other vars yet, but as soon as I do so I'll merge this.

I'll then consider if the renaming issue is worth filing

@Coolguy3289

This comment has been minimized.

Copy link
Collaborator

Coolguy3289 commented Oct 8, 2019

Just confirmed through Github repo search that vars only existed in the two files mentioned above. LGTM.

@nikooo777 nikooo777 merged commit 6ace1d8 into lbryio:master Oct 8, 2019
1 check failed
1 check failed
Travis CI - Pull Request Build Failed
Details
@tzarebczan

This comment has been minimized.

Copy link
Member

tzarebczan commented Oct 15, 2019

@haichuan0424 , thanks for the PR!

Can we show you some appreciation for the contribution?

Also, we're giving away some Hacktoberfest bonuses and goodies for this month. We'll get it all figured out after you shoot us an email after this is reviewed/merged.

@Coolguy3289

This comment has been minimized.

Copy link
Collaborator

Coolguy3289 commented Oct 15, 2019

Already merged

@haichuan0424

This comment has been minimized.

Copy link
Author

haichuan0424 commented Oct 23, 2019

@tzarebczan
Glad I could help! It would be great to get some bonuses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.