ethereumjs / ethereumjs-blockstream

Reliable stream of Ethereum blocks
MIT License
80 stars 19 forks source link

Format code with Prettier #21

Open fabioberger opened 6 years ago

fabioberger commented 6 years ago

I've needed to read through the codebase while debugging an intermittent failure and found it much easier to read after formatting it with Prettier.

Would really appreciate it if it could be used from now on. It's really great at taking away the burden of consistent formatting away from the developer, similar to Go-fmt in Golang.

It can be run with:

npm run prettier
MicahZoltu commented 6 years ago

I am amenable to a subset of these changes, particularly the ones that address some inconsistencies. However, for many others they are contrary to my personal coding style so while they may make the code easier for you to read, it makes the code harder for me to read. Since I'm the primary maintainer, I preference "easy for me to read" over "easier for you to read.

Addressing each specific change proposal

Inclusion of a linter

I'm on the fence on including a linter. I understand why they are valuable, usage of it would have prevented me from committing some inconsistencies, but at the same time I find that tuning a linter to lint the way you want can be a huge PITA and it is often ugly when you need to defy the linter occasionally (usually requires a comment in the code to supress the linter). I think if someone else does the work of tuning the linter to match my preferred coding style I would consider accepting a PR for it, though I would want it included in CI as a gate (after compilation).

Spaces over Tabs

As can be seen in the .editorconfig file (https://editorconfig.org/), this project uses tabs for indentation, not spaces. If you like, I can explain why I believe this to be superior to spaces but I suspect that won't go anywhere since many people consider this a religious debate.

Hard Wrap @ 120 characters

I'm pretty strongly against hard newlines. If you don't like long lines, use an editor that soft wraps. This results in a responsive viewing experience which any UX designer will tell you is superior to a hard-coded viewing experience. If I'm on a display that only shows 115 characters and has soft-wraps on (e.g., viewing a diff on a mobile device) hard wraps will completely destroy my view, while soft wraps will work fine for me. I believe all major editors support soft wraps. Also, if a line truly is long, that sometimes can be a code smell. There are exceptions to this however, like constructing an immutable object or function signatures for pure functions that take in all of their dependencies. We can debate the merits of one-lining these exceptions or not, but the problem circles back to the linter problem, where exceptions to the rules of a linter can result in even uglier code (by some standard).

Trailing comma

I'm generally a fan for multi-line objects like

const foo = {
    bar: 5,
    baz: 6,
}

but tend to be against for single-line objects like const foo = { bar: 5, baz: 6 }. I might be able to be convinced to use trailing commas in the single line case, as I can see the value even if it doesn't come naturally to me.

Single Quote

This project probably was mixed previously, I'm in the single quote camp at this point mainly because typing single quotes in marginally easier than typing double quotes and I don't have any stronger argument for one or the other.

I'll comment on some of the others not specifically specified in the prettier file inline in the PR.

MicahZoltu commented 6 years ago

I can certainly appreciate the attempt at changing the style to one that you find easier to read, and I definitely encourage people to work with code styled in their favorite way. In fact, often times when reading other people's source code I'll start by reformatting it to match my preferred style before reading it, and if I want to submit a PR it is much simpler to do so if the repo matches my preferred style.

However, as is I am very unlikely to merge this PR since it changes the style to one that I have a difficult time reading. If you are interested in modifying this PR to include a subset of the changes per discussion above, or if you are interested in using this PR as a forum for further discussion on why I prefer one style over another (I actually do have arguments for each thing, not just "because I'm used to it") I'm open to it.

fabioberger commented 6 years ago

Thanks for the detailed feedback @MicahZoltu. Although this PR may seem like me trying to impose my conventions on your project, I hope to convince you that this is not the case.

I don't care what the conventions are, as long as they are easy for everyone to adhere to. As an open-source project, it would be in everyones best interest if the contributor and the reviewer do not need to think of & discuss stylistic conventions, and instead can focus on the substance of PRs.

Prettier is not a linter, rather it's an auto-formatter, and it can be adjusted. I've already gone ahead and made the following changes at your behest:

This took me 2 seconds. Change the config, re-run prettier. I've also added a prettier check to Travis CI, so formatting will be enforced for all future PR's.

There were some things I was not able to easily change to your existing conventions:

But I think that the trade-off here is that you give up some of your undocumented conventions and preferences in return for guaranteed consistency and saving every contributor the trouble of learning a new set of conventions. Rather, they can simply hit "save" and let the computer worry about formatting. I really hope you'll reconsider this PR.

MicahZoltu commented 5 years ago

I took another look at this PR today and I think the only contentious part is line wrapping. Is it possible to tell Prettier to not do line wrapping but apply the rest of its changes? If so, I would propose that we do a PR that does everything except line wrapping and then we can discuss the line wrapping stuff in isolation, with a smaller changeset (allowing us to see more clearly what is being discussed).