lionsharecapital / lionshare-desktop

🦁 Simple cryptocurrency price and portfolio monitor for macOS
https://lionshare.capital
MIT License
568 stars 76 forks source link

Format code with prettier #58

Closed jessepollak closed 7 years ago

jessepollak commented 7 years ago

This PR formats all our JS logic with prettier, reduces the intensity of our eslinting rules, and adds a pre-commit hook that formats and lints code (and if anything fails it will not commit).

I'm opening this PR because I'm a big fan of linters and have them enabled in my editor, and while lionshare-desktop seems to have a good set of linting rules defined, they seem pretty un-uniformly enforced. I figured it would be useful to start standardizing that enforcement and make it easier for everyone to lint their code. I'm not at all attached to this logic shipping, but I figured I'd start a conversation!

The primary changes in this PR are:

  1. Formatting all code with prettier. Unlike tools like eslint which lint code, prettier consistently formats code so there's no opportunity to even write code that is outside the format.
  2. Add pre-commit hooks that format, and then lint with eslint, any code that is going to be committed.
  3. Significantly reduce the intensity of our eslint rules. After looking at what it would take to bring the entire codebase up to our current rules, it seemed out of scope for this PR and like something that we would want to as a group commit to before any more work was done.

If this PR were to be merged in, I would recommend taking the following next steps:

  1. Setting up a .eslintrc file for the desktop/ source so that logic can be linted (it is just formatted with prettier right now).
  2. Re-enabling the airbnb javascript eslint rules and working as a group to refactor all of our logic to meet the standards (this is mostly just a lot of tedious code changes).

This is a really big diff, but I wanted to make sure that before I made a suggestion, I ran through what it would take to get started with this approach! Let me know what y'all think :)

jorilallo commented 7 years ago

@jessepollak Are you reading my mind? I been looking into prettier and was about to adopt it so happy to see this PR. I haven't enforced airbnb eslint rules too much on this project but in general like them a lot. I agree that prettier is a good direction to take, both for productive and simplicity, so happy to move into it.

How would you feel about adding trailing commas in? I'm usually a fan because it makes writing new lines easier and less prone to syntax errors. Not sure if there's a way to remove semicolons on ES6 class methods but that's less of an issue, mainly visual.

jessepollak commented 7 years ago

maybe 😏

I'm down with trailing commas - I'll make that change and push back up. I don't think there's a way to configure semicolon usage.

jorilallo commented 7 years ago

🙏

jenbennings commented 7 years ago

Love this.

jessepollak commented 7 years ago

@jorilallo done - give it a run through and make sure everything works ok -- if that checks out, we can open up 2 more issues with my next steps and get this sucker merged in :)

jessepollak commented 7 years ago

OK, so it looks like with the trailing comma setting enabled, the server code won't compile because it adds commas in places that aren't compatible with ES5. It seems like the logic to support ES5 only has been added here but not yet shipped in a new release. We may need to wait for that to ship before we want to merge this in.

jorilallo commented 7 years ago

just noticed it myself. Maybe just remove the setting for now and we can add it back in later if we feel like it. Not feeling too sentimental right now

jessepollak commented 7 years ago

@jorilallo just updated the package.json to use prettier@master and it's working fine - you cool with that for now?

jorilallo commented 7 years ago

I'm cool, 🦁 is cutting edge

jorilallo commented 7 years ago

@jessepollak once you're ok with this, feel free to merge

jessepollak commented 7 years ago

@jorilallo I don't have permissions to merge yet 😞