NebulousLabs / Sia-UI

A Graphical Frontend for Sia - https://sia.tech
MIT License
389 stars 109 forks source link

Prettier Standard formatting #795

Closed eddiewang closed 6 years ago

eddiewang commented 6 years ago

This should be merged before #794 so I can rebase that branch with master. Thanks to @MSevey for the idea.

This PR is the simply the result of running prettier-standard 'js/**/*.js' 'plugins/**/*.js' in the repo.

Standard JS (https://github.com/standard/standard) is a popular style guide. I've used quite a few (Airbnb, Github, Google) and I always find myself coming back to Standard.

Prettier (https://github.com/prettier/prettier) is an auto-formatting tool that keeps code style consistent.

lukechampine commented 6 years ago

this mostly looks fine to me. It's a bit aggressive on splitting up long lines (which traditionally we don't care about so much), and I also dislike indenting case blocks in switch statements; is the formatter configurable? I imagine we'd want to add it to our package.json somehow for easy invocation?

eddiewang commented 6 years ago

The formatter is configurable - the scripts exist in package.json under npm run format as well as some git hooks on the larger PR, but for this one I just manually ran the script. I'm currently working on some failing tests that still exist on master. Once that's done, I'd like to rebase this PR so that Travis builds work properly again, so hold off on merging this for now.

eddiewang commented 6 years ago

@lukechampine I removed the unnecessary whitespacing. You are right. Lots of whitespace that was accidentally being left in the HTML. But that's also why I prefer using {' '} for whitespacing in JSX - it makes it really clear that you have spaces in text.

The default width for long lines in Standard JS is 80 chars. I'm inclined to stick with the defaults (look at how clean the eslintrc is compared to the old one) for now, just because it's a well-known standard that people are used to. It's super easy to change that in the future, we just add a custom rule and run npm run format.

I also updated the babelrc so that webpack builds properly, and so that tests passed based on my fix from yesterday's PR.

I've rebased from master and this PR should be good to go.

DavidVorick commented 6 years ago

Is it able to distinguish between text lines and code lines?

Not a big deal / blocking factor for me either way. I would probably target 80 chars for text lines (since readability of a broken up line is not impacted) and 160 chars for code lines (since readability takes a huge hit if you split up a code line - unless you do it very elegantly, which I don't imagine a script is able to do.