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

[prettierx] --paren-spacing option from WordPress #16

Merged
merged 13 commits into from
Feb 27, 2019

Conversation

brody4hire
Copy link
Owner

Nice option that is cherry-picked from the WordPress fork of Prettier ref: prettier/prettier#5919, with a couple lint fixes and some test coverage added.

Note that the alignObjectProperties option is known to not work quite right with this new option enabled. I hope to get this fixed someday.

/cc @jsnajdr

jsnajdr and others added 13 commits February 27, 2019 13:55
(whitespace) according to WordPress style rules

(with a quick lint fix by @brodybits)

Add description of the changes from the WordPress fork to the start of
README.md

Update test snapshots to recognize the paren-spacing option in
tests_integration

Co-authored-by: Jarda Snajdr <[email protected]>
Co-authored-by: Christopher J. Brody <[email protected]>
…xpression or MemberExpression

Before:
```js
! (
  a || b
 )
```
After:
```js
! (
  a || b
)
```
Fixed by inserting the parens directly when formatting a Binary or LogicalExpression instead
of relying on `genericPrint` inserting them depending on result of `path.needParens`.

Also fixes parens about MemberExpression object:
```js
(
  a || b
).c;
```
and semicolon insertion in no-semi mode for MemberExpressions:
```js
x
;(a||b).c
```

All unit tests continue to pass (including the Flow and Typescript ones).
(with a quick lint fix by @brodybits)
cover parenSpacing in tests/space-before-function-paren/tslint-compat
and cove babel parser in tests/empty_statement
@brody4hire brody4hire merged commit 79db0ba into dev Feb 27, 2019
@brody4hire brody4hire deleted the prettierx-paren-spacing branch February 27, 2019 20:36
@jsnajdr
Copy link
Contributor

jsnajdr commented Feb 28, 2019

Thanks for making the option more widely available @brodybits 👍

Note that the alignObjectProperties option is known to not work quite right with this new option enabled. I hope to get this fixed someday.

Yes, interactions between options are sometimes fragile and tricky to debug. I think that's the technical reason behind the Prettier's dislike of options. They don't compose so easily like, e.g., ESLint rules.

@brody4hire
Copy link
Owner Author

Interesting analysis, thanks.

In this case the alignObjectProperties option needed a workaround solution, which makes it especially fragile. I found another issue yesterday (#19), don't know when I will get a chance to get these fixed. I am now taking the liberty to rant in this closed PR:

I think Prettier should have been based on something like Babel, with a customized output printer that is more flexible and maybe some other custom plugins. This is analogous to the way "Standard JS" is based on ESLint, with a couple new rules in its own ESLint plugin. In fact, the output printer should have been in smaller parts in a similar fashion to the way custom Babel parsers can be dynamically run together.

I discovered goto-bus-stop/babel-plugin-generator-prettier which can be used to run Prettier on the output only. It has a very interesting trick in: https://github.com/goto-bus-stop/babel-plugin-generator-prettier/blob/master/index.js#L20-L27

The trick is nice work but is a very sad reflection on the design of Prettier. My thoughts on a better design are in #8 (comment):

I think the ultimate design would be a pipeline-based toolset which uses the Acorn parser data structure, which seems to be how Babel JS works. I really like the general design of remark (https://remark.js.org/), which does Markdown processing in a set of plugins and is based on unifiedjs (https://unified.js.org/).

A step further may be to use a Flow Based Programming paradigm, like they did in noflo/noflo-jekyll (see this article and duplicate article on dzone).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants