[RFC] Apply Prettier Formatting

[marvinchin]

#746

Attempt to use Prettier to format working files.

As recommended in the Prettier docs, I've disabled all formatting related eslint rules (Prettier will take care of formatting). Code quality rules are still retained.

You can use Prettier plugins (available on most IDEs) to automatically format your code while coding 🙂

Do let me know what you guys think about this, and whether or not we should go ahead with it!

EDIT:
Just to note, this PR is to show how Prettier formatting would look like, and is not meant to be merged into master

[acjh]

If I had not known that you used Prettier, I would be pretty convinced that this was malicious vandalism.

Some of it makes sense, but way more of it is just plain ugly if not downright violating common coding standards (in our case, Airbnb's). It doesn't even seem to be consistent about the handling of statements with ternary operator (fs-extra-promise.js vs index.js).

I think gradually removing some of our Eslint overrides (i.e. going towards Airbnb's coding standard) if they make sense and improve ease of development is generally good. However, opinionated formatting has no end.

Please forgive my strong wording, but I find this PR offensive.

[acjh]

Linting this file is intentional. It is ignored by default, so this line would be redundant anyway.

[marvinchin]

Without this line npm run lint seems to be picking up .eslintrc.js on my machine, not sure why it doesn't seem to be consistent.

[acjh]

Avoid changing formatting and config in the same commit unless syncing configs.
Also, this violates the Eslint rule overrides at the top of the file.

I kept double quotes as their documentation uses that and many projects do too, so it's easier to copy rules as-is.

[acjh]

Why would you even commit and push this?

[marvinchin]

I did npm install using npm 5 as you suggested here, and this was the output generated.

[acjh]

v5.10.0?

[marvinchin]

Yes. On further investigation the issue seems to be reported here, though for a different npm version.

[acjh]

To clarify, I found the disregard of this comment by this PR offensive and felt strongly about it:

Do we intend to adhere to the current eslint rules?

Our current eslint rules must be followed.

I understand now that you spoke to @yamgent about it in person. Please post a brief update there.

Thank you for your feedback and I will work on improving my tone.

[acjh]

As recommended in the Prettier docs, I've disabled all formatting related eslint rules (Prettier will take care of formatting).

I like that tooling would remove ambiguity, just not the choice of formatting rules.

The prettier plugin overrides many Eslint rules from Airbnb. Prettier should be used to format based on the project's rules, not be allowed to freely define the formatting rules of the project.

[marvinchin]

To update on our conversation with @nicholaschuayunzhi and @yamgent today:

We were discussing how to make Prettier play nice with our existing eslint rules, and the decision was to remove the formatting rules first to let the team observe how Prettier formatting works on its own and see if those are to the team's preference (hence this PR).

I apologise if there was any confusion regarding the intent or context of this PR - I'll include updates on any related offline discussions in the future to avoid ambiguity.

[marvinchin]

Also, to give a little more context as to why it was tricky to make prettier work alongside our existing eslint configs:

Prettier is an opinionated code formatter, so it does not follow the formatting rules defined by eslint, but instead assumes full responsibility for the formatting of the code. This naturally means any formatting related eslint rules will no longer be relevant if prettier is used (code quality rules are still useful).

As @acjh pointed out, this would mean letting prettier define the formatting rules of the project. The advantage to this is that it removes the cognitive overhead and ambiguity in thinking about how code should be formatted, but we lose the ability to define more granular rules for formatting if they differ from prettier's way of formatting code.

There are other ways which we can try to use to make prettier work with eslint (e.g. prettier-eslint), but IDE support for these are less extensive at the moment.

[nicholaschuayunzhi]

I apologize for not stepping in to clear up the confusion earlier.

I too was surprised by how aggressive Prettier was. I do agree with @acjh, it makes more sense for Prettier to enforce our current eslint rules if possible - many of the configurations made by different developers were purposeful. I apologize for not thinking too deeply about that. However, that being said, we would not have been able to make a well informed judgement without the PR by @marvinchin, so much thanks for trying it out!

[damithc]

Looks like this is not going to be an easy decision as there are pros and cons. In that case shall we maintain the status quo? Especially because this doesn't directly value add to our immediate goal (i.e., V2)? While it may be possible to tweak prettier to a level we are all OK with, the required effort may distract our attention away from the goal at hand. We can always revisit it again at a less-pressure time.

Thanks @marvinchin for experimenting with prettier and others for the feedback.

Other possible takeaways:

• Remember to update GitHub threads with a summary of what was discussed off-line or using other channels. Even slack discussions may not be visible to future contributors.
• 'Opinionated' tools are better adopted at the start of a project. They are harder to adopt in ongoing projects as it is unlikely that every project member's opinion will agree with the tool's.

[marvinchin]

Thanks everyone for the inputs! 🙂