savoirfairelinux / ringme.js

A library to display a « Ring Me » button on a website.
https://frama.link/ringme
GNU General Public License v3.0
4 stars 14 forks source link

Standardize code using ES6 identifier types #29

Closed samnegri closed 6 years ago

samnegri commented 7 years ago

Expected Behaviour

Use const type to declare identifiers that should not change it's value. If there is no way to keep identifier immutable, use let type to declare identifiers that should change it's value and avoid using var type

This are not rules, but guidelines, use your own judgement to identify the best type for each scenario.

More clarification here

Actual Behaviour

At the moment all identifiers are using var

Steps to reproduce

Just check the source code :smile:

nabrahamson commented 7 years ago

I can tackle this. It also may be smart to add some eslinting rules to maintain this change in the future.

ventilooo commented 7 years ago

Hi @nabrahamson, Thanks for your interest in this project. I'll create en issue for the Eslinting. would you be assign to this one and the Eslint one ?

nabrahamson commented 7 years ago

@samnegri What are your requirements for having this issue completed? I am running your test .html page in a browser, and it appears that you don't have babel or webpack set up to browserify your source .js files.

nabrahamson commented 7 years ago

@ventilooo I'd be happy to take on setting up linting for this project.

samnegri commented 7 years ago

Hello @nabrahamson, thank you for contribute!

Yes, the project is not currently using babel or webpack, but this does not block this issue. I belive you refer to the tests/ringme.html file, is it right? It does show an error in the console when the page is loaded that says module is not defined but that's a other issue that I think issue #5 should solve this. It would be great if you could help with this other issue.

I also checked your PR and I belive you got what this issue is about, there are just a few adjustments before we can merge, please check my comments

samnegri commented 6 years ago

After the last merges, this PRs needed to be update and since there was no more feedback about the last reviews I opened a new one updated with trunk.

@ventilooo and @ssirois, can you assign someone to review? I'd like to solve this before a new synchronization is needed.

ssirois commented 6 years ago

@samnegri, @nabrahamson and @combmag: i believe we now have three PRs addressing this issue.

33, #38 and #43

All three fix this issue, but #43 is more in sync with the trunk as I am writing this.

I would kindly suggest that I should merge #43 and give credit to all three of you in the merge by using the Co-Authored-By: convention (see https://git.wiki.kernel.org/index.php/CommitMessageConventions).

I do believe it's the fair thing to do.

Any objections?

samnegri commented 6 years ago

I agree.

ssirois commented 6 years ago

I have manually merge #43 in order to add credit where it belonged in this team effort.

Thanks to @samnegri, @nabrahamson and @combmag for all the work that gave us the possibility to close this issue rapidly and efficiently.

I will close PRs that have been deprecated by this manual push.

image