search

henryboldi / felony

🔑🔥📈 Next Level PGP
MIT License
3.47k stars 130 forks source link

Updated .eslintrc and fixed a whole bunch of linting errors and warnings #67

Closed greg-js closed 8 years ago

greg-js commented 8 years ago

So I noticed quite a bit of linting errors and warnings so I went through them, fixed the obvious ones, addressed some less obvious ones and updated the eslint config files for those cases where the rules didn't seem to reflect the style choices used in the project.

I tried to stick as close as possible to the style used and I'm pretty sure I didn't break anything. But hey, with changes like these, accidental breakage is possible so you should probably go through the changes before accepting the PR. If I don't fall asleep later I'll go through it myself and add github comments wherever a certain change may seem controversial.

Even with these changes though, npm run lint still finds three errors and a warning. One of them (the one in ./app/reducers/keychainReducer) I wasn't sure how to handle. The others I'm assuming are a result of as of yet unfinished features?

Also, I understand that stylistic changes can be very personal and controversial. It's by no means my intention to enforce any kind of style on anyone -- honestly, if it were up to me, there would be about 1000000 more semicolons in this project. I just want to help you get closer to a perfect state of no-linting-errors development bliss. If you don't like the spirit of this PR, just close it, I have no feelings so they can't get hurt.

Oh, and can you tell that a) I really like the idea behind this project, and b) I'm currently funemployed? ;-)

P.S. I realize I've probably been bombarding your inbox by adding comments on the changes but I regret nothing

greg-js commented 8 years ago

Well it seems that somewhere along the way, an error crept in and the composer now pops up when hovering over the floating button. I need sleep though so I'll check what went wrong in the morning, but do not merge yet! :-)

greg-js commented 8 years ago

So in the end putting the binding in the constructor caused issues, and using an arrow function in render left the linting error, so I added a workaround by factoring out the floating button div (and rolled back the change in the eslint sorting order). So the bug is fixed and now down to three linting problems.

henryboldi commented 8 years ago

Excellent! Thanks for posting updates on your progress 😃

greg-js commented 8 years ago

Cool, thank you for the pointers, will use your comments to update the PR later today or tomorrow.

greg-js commented 8 years ago

Uhh, I'm not sure what happened there, it seemed fine on my end until I pushed it. Well, I could create a new, clean PR later with these linter-pleasing changes if you'd like. I'll wait until you've looked at it though.

Remaining linting issues are:

casesandberg commented 8 years ago

Thanks @greg-js, I plucked out your changes and merged them into master!