Standardize code

[nicolasferraro]

Once #59 is merged, we should start fixing the things that eslinter points out.

Most of the errors can be autofixed by running the linter with the --fix parameter. The remaining ones will have to be fixed by hand

---

Two places to change: 1) npm run-script lint in .travis.yml file should be uncommented, so our CI will run the linter 2) this commit should be reverted, so the linter runs before every commit

And then we need to go through every file and make sure it passes the linter :)

[mcoirault]

Hey and happy Hacktoberfest.

I'll take care of this.

[cannawen]

Awesome, thanks for volunteering @mcoirault

[cannawen]

@mcoirault FYI we have just added a CONTRIBUTING.md doc, please check it out when you have time! Sections "Etiquette", "Work on an issue" and "Make a PR" are most important

[mcoirault]

Yes, I've read that doc. I also saw about the 7 days limit. I might need some more time : busy days prevented me from working on the bot as much as I wanted. 😄

I might also need some help with testing : I'm not 100% confident that some of the changes es-lint wants us to enforce are going to work.

[cannawen]

No problem at all, feel free to take more time! Thank you for updating us :)

What do you mean by testing might not work? Can you give an example?

[cannawen]

I've updated the instructions to include more details on what needs to be done :)

[mcoirault]

I'm slowly getting there. There are a few issues where I will need help.

First, I'm not familiar enough with mocha, so I didn't find a way to make it handle the comma-dangle rules we need to enforce. When trying to run npm test, it stops at and displays this :

.\metric_units_reddit_bot\test\bot-test.js:22 ); ^ SyntaxError: Unexpected token )

Secondly, I'm having trouble tackling the last 6 eslint issues in test/bot-test.js. Eslint claims some variables are unused even though they are.

Once this is done, I'll have to run mocha and figure out what broke in the process of linting. 😄

You can check the ongoing work on my repo branch. I'm not sure what's the best way to share it : should I make a PR already or is a link fine?

[cannawen]

@mcoirault What version is your node? It needs to be >8.0 in order for some of the linting to work

[mcoirault]

Dang, my node version was 5 something. MY bad. I need to be a better dev, I need to update my tools more...

It fixed the dangling comma issue. I'm now facing a different issue. I'm not sure if it's linked to my dev environment being windows. I'll investigate.

`C:\wamp\www\metric_units_reddit_bot\node_modules\mocha\lib\mocha.js:79 this.files = []; ^

TypeError: Cannot set property 'files' of undefined at Mocha (C:\wamp\www\metric_units_reddit_bot\node_modules\mocha\lib\mocha.js:79:14) at Suite.describe (C:\wamp\www\metric_units_reddit_bot\test\converter-test.js:20:5) at Object.create (C:\wamp\www\metric_units_reddit_bot\node_modules\mocha\lib\interfaces\common.js:114:19) at context.describe.context.context (C:\wamp\www\metric_units_reddit_bot\node_modules\mocha\lib\interfaces\bdd.js:44 :27) at Suite.describe (C:\wamp\www\metric_units_reddit_bot\test\converter-test.js:19:3) at Object.create (C:\wamp\www\metric_units_reddit_bot\node_modules\mocha\lib\interfaces\common.js:114:19) at context.describe.context.context (C:\wamp\www\metric_units_reddit_bot\node_modules\mocha\lib\interfaces\bdd.js:44 :27) at Object. (C:\wamp\www\metric_units_reddit_bot\test\converter-test.js:18:1) at Module._compile (module.js:624:30) at Object.Module._extensions..js (module.js:635:10) at Module.load (module.js:545:32) at tryModuleLoad (module.js:508:12) at Function.Module._load (module.js:500:3) at Module.require (module.js:568:17) at require (internal/module.js:11:18) at C:\wamp\www\metric_units_reddit_bot\node_modules\mocha\lib\mocha.js:231:27 at Array.forEach () at Mocha.loadFiles (C:\wamp\www\metric_units_reddit_bot\node_modules\mocha\lib\mocha.js:228:14) at Mocha.run (C:\wamp\www\metric_units_reddit_bot\node_modules\mocha\lib\mocha.js:514:10) at Object. (C:\wamp\www\metric_units_reddit_bot\node_modules\mocha\bin_mocha:480:18) at Module._compile (module.js:624:30) at Object.Module._extensions..js (module.js:635:10) at Module.load (module.js:545:32) at tryModuleLoad (module.js:508:12) at Function.Module._load (module.js:500:3) at Function.Module.runMain (module.js:665:10) at startup (bootstrap_node.js:187:16) at bootstrap_node.js:607:3`

[cannawen]

I checked out your branch (the link is fine, no need to PR 😃 ), and was able to run npm run lint without any errors. Is that the command you were running? I am on a mac though, so yeah, it could be a windows thing :/ Google wasn't super helpful with the error.

[cannawen]

@mcoirault Can you try deleting your node_modules folder, and then npm install again? I don't see this.files = []; on line 79 of my mocha.js, so this might be a versioning issue from the node upgrade

[cannawen]

Also, I noticed that your fork is now pretty far behind master. Not sure if doing all the fixes at once is desirable because of all the conflicts that will occur - I wonder if it would make more sense to turn all the errors into warnings, and fix them slowly over time, and slowly turn the warnings back into errors? Or perhaps the conflicts won't be so bad? Dunno, do you have any thoughts on this?

[mcoirault]

I'm confident I can handle bringing the commits back in my branch, I'm getting more and more familiar with the code base.

Imho, I think it would make sense to fix it all at once and do some thorough testing to make sure nothing major breaks. Doing fixes overtime might pollute the history by having every PR fixing a little bit of the code base. If we can pull it off, doing it all at once will just be a nice milestone to start fresh.

Lowering errors to warnings would make sense as to not hinder the other commits whilst we finalize this.

npm run lint is running fine. It gives me the remaining errors that I know I need to fix. It's npm test that is posing issue. I nuked /node_modules/ and run npm install again (node8.7.0 and npm5.4.2) but I'm still getting the same error :(

[nalinbhardwaj]

@mcoirault just to make sure the issues with npm test don't arise due to windows related issues, you can try running your branch on travis CI.

To do that, you can

1. Just make a PR(the build will show up in one of the checks)
2. Or just login with GitHub on the site and switch on builds for your fork.

[mcoirault]

@nalinbhardwaj Thanks for the heads up.

Rebasing my branch to the current master has solved the npm test issues I had. I'm not sure why. I'm now left with the task of fixing all the things I broke here and there 😄

[cannawen]

Glad your issue was magically fixed :D haha

[anirudhbs]

I want to take up this issue.