mozillascience / PaperBadger

Issuing badges to credit authors for their work on academic papers
https://badges.mozillascience.org/
Mozilla Public License 2.0
95 stars 45 forks source link

Initial upgrade to latest mofo-style version #168

Closed josmas closed 8 years ago

josmas commented 8 years ago

This is a first step towards supporting the latest version of mofo-style as per #166

At the moment it uses a modified .eslintrc.yaml file, copied from the module and provided in the root folder, as a way to start the transition. It is a compromise, but fully supporting mofo-style would mean fully migrating to ES6, and it's a bit too much to do in just one step.

There are a number of things that can be done after this initial step, for instance, hook in babel presets and start using back-ticks instead of quotes (for strings), modify all variable names to camel case, or start using arrow functions instead of normal definitions. The final stage would be to completely get rid of the yaml file in the root folder and use the one in the module.

josmas commented 8 years ago

Forgot to add; I purposely left 6 errors uncorrected. I thought they were important enough to be picked up one by one and refactored separately. For instance, this is something that appears in several files:

getBadges is defined in both line 27 and line 32, which are in the same scope. A good way to proceed here would be to write a unit test around the functionality before refactoring it, to make sure all is fine. But this needs to be looked at more carefully.

abbycabs commented 8 years ago

Hey @josmas! Thanks again for this -- really fantastic! Changes so far look good.

I'll create issues for the remaining errors. I'd rather leave this un-merged while we still have errors (so we keep a working linter!).

abbycabs commented 8 years ago

https://github.com/mozillascience/PaperBadger/issues/169

josmas commented 8 years ago

Hey @acabunoc thanks for the review. I have pushed 3 new commits with fixes for the 6 errors. Explanations in the commit message. This should fix #169 (all going well).

Didn't add any new tests because the existing ones already use some of this functionality, and the changes were not as deep as I first thought they would be (mainly renaming).

abbycabs commented 8 years ago

Your eslint custom rules makes sense! I left a suggestion for further refactoring of getBadges -- there's a lot of common code between the two defined functions in both papers/index and users/index.

Thanks @josmas :) I have a surprisingly meeting free late afternoon today. Happy to help out / take a look.

josmas commented 8 years ago

Hi @acabunoc I do agree on the refactoring bits, but I would like to keep things separated. If we keep these changes as linter modifications, and work on a different pull request for refactorings, we could revert specific commits if at some point any issues are identified. I like keeping things separated (kinda planning ahead!). Also, for bigger changes, I'd like to beef up the tests first. What do you think?

abbycabs commented 8 years ago

Good point -- forgot that this has expanded quite a bit already :) R+ merging and creating new issue for the refactor

janhohner commented 8 years ago

:thumbsup:

josmas commented 8 years ago

Thanks, @acabunoc!