Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider using Prettier for formatting rules #206

Closed
agubler opened this issue Oct 1, 2017 · 6 comments
Closed

Consider using Prettier for formatting rules #206

agubler opened this issue Oct 1, 2017 · 6 comments

Comments

@agubler
Copy link
Member

agubler commented Oct 1, 2017

Since version 1.2.0 Prettier supports TypeScript as a parser. We should consider including Prettier into the Dojo CI pipeline for formatting rules to help enforce a consistent code style across the code base.

We already have a style guide for Dojo 2 but some of the rules are not enforceable with TSLint which means they need to be highlighted through code review, using Prettier's opinionated formatting styles (and updating the style guide to reflect this) in combination with some of the latest, more granular TSLint rules means that contributors can easily format their code in accordance this standard.

Using plugins such as https://github.com/ikatyang/tslint-plugin-prettier means that Prettier errors will be reported by TSLint and fail using grunt dev / grunt dist.

@kitsonk
Copy link
Member

kitsonk commented Nov 8, 2017

Adding notes from dojo/widget-core#700...

Ok, I have looked at it again and actually the item I objected to was actually contrary to our style guide.

The things that I have issues with:

  • Cuddles type assertions (e.g. <any>foo instead of <any> foo). Though we should be avoiding that syntax and instead use foo as any.
  • Removes space from literals (e.g. [foo, bar] instead of [ foo, bar ])
  • Does not start certain blocks on a new line (e.g. } else { instead of else {)
  • Does not brace single argument lambda/arrow functions (e.g. foo => foo.bar instead of (foo) => foo.bar).

Of all of them though, it is only the last one that I really have an issue with, and it seems I am not the only one as prettier/prettier#812 is a request. The current status is that they are going to consider it because of the amount of feedback, though they have locked the issue. Since it feels like it is something they will eventually deliver, I would not object to using prettier, though I would want to adopt that flag as soon as it becomes available.

@agubler
Copy link
Member Author

agubler commented Dec 15, 2017

Prettier 1.9.0+ added the option to ensure params are added to single argument arrow functions, which resolves @kitsonk's biggest concern from the initial POC.

From here, I think we are at a point where we can start rolling the changes across the packages.

@agubler
Copy link
Member Author

agubler commented Dec 15, 2017

Packages:

@kitsonk
Copy link
Member

kitsonk commented Dec 15, 2017

Prettier supports adding config as prettier to the package.json. Do you think we should configure it in that fashion?

@agubler
Copy link
Member Author

agubler commented Dec 15, 2017

@kitsonk my thoughts exactly and have already added that to the widget-core conversation (just hadn't pushed)

@dylans
Copy link
Member

dylans commented Mar 2, 2018

All done, thanks everyone for making things prettier (sadly pun intended)!

@dylans dylans closed this as completed Mar 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants