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

Keep decorators inline if they were written inline #5188

Merged
merged 6 commits into from Oct 8, 2018

Conversation

Projects
None yet
5 participants
@duailibe
Copy link
Member

commented Oct 4, 2018

Implements @lydell's suggestions: #4924 (comment)

Closes #4924
Closes #5088

What this PR does is select the separator for each decorator by checking:

  • is there a new line between the start of the first decorator and the start of the last decorator
  • is there a new line right after the end of the last decorator

If either of those are true, we should use a hardline separator, otherwise it's a line. If they are separated with line they will be the in the same line, unless they don't fit, then they'll break; hardline will force a break.

"Breaking", in this case, means each decorator is printed in a separate line and the thing they decorate too.

I'll navigate to the threads now to see if I can't find any case not covered in our tests.

duailibe added some commits Oct 4, 2018

@duailibe

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2018

I believe our current test suite covers all the cases, this is ready for review

@duailibe duailibe requested review from suchipi, lydell and ikatyang Oct 4, 2018

@j-f1

j-f1 approved these changes Oct 4, 2018

@lydell

This comment has been minimized.

Copy link
Collaborator

commented Oct 4, 2018

Looks good!

I played around in the playground and … I'm not sure what I found.

Prettier pr-5188
Playground link

--parser babylon

Input:

// Is this inline or multiline?
@foo(
  1
) class A {}

// Should these be consistent?
@foo @bar export default class B {}
@foo @bar export class C {}
@foo @bar class D {} // for comparison

// Should we even force multiline for class and function *declarations*
// (but not methods, properties and params)?

Output:

// Is this inline or multiline?
@foo(1) class A {}

// Should these be consistent?
@foo
@bar
export default class B {}
@foo
@bar
export class C {}
@foo @bar class D {} // for comparison

// Should we even force multiline for class and function *declarations*
// (but not methods, properties and params)?
@duailibe

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2018

// Is this inline or multiline?
@foo(
  1
) class A {}

My intuition is that it should be multiline (i.e. that's a bug in the implementation), but I'm not sure.

// Should these be consistent?
@foo @bar export default class B {}
@foo @bar export class C {}
@foo @bar class D {} // for comparison

I think so, but I'm not sure.

// Should we even force multiline for class and function *declarations*
// (but not methods, properties and params)?

That sounds reasonable to me. And this fixes the above case, as well. I'll change it 👍


EDIT: Function declaration can't be decorated, so I guess we'll force only for class declarations

@lydell

This comment has been minimized.

Copy link
Collaborator

commented Oct 4, 2018

Nice! What about function declarations in addition to class declarations?

@duailibe

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2018

@lydell I don't think function declarations can be decorated. https://tc39.github.io/proposal-decorators/#sec-updated-syntax

Am I missing something?

Also, what about class expressions?

const foo = @deco class {}

I forgot about them in this PR, but it seems to me we can allow inline in that case?

@lydell

This comment has been minimized.

Copy link
Collaborator

commented Oct 4, 2018

I don't think function declarations can be decorated.

Oops! Sorry, you're right.

Also, what about class expressions?

Cool, I didn't know that was valid. Currently it looks a bit weird when multiline:

Prettier pr-5188
Playground link

--parser babylon

Input:

const foo = @deco
class {}

Output:

const foo = @deco
class {};

Maybe we should not look at the source for those (always inline?) (and make sure that they break nicely when too long)?

@duailibe

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2018

We could not look but then we could have a @deco(verylongname) that would break either way. I guess we just need to print it better when breaking

const foo = 
  @deco
  class {}

What about this?

@suchipi

suchipi approved these changes Oct 4, 2018

Copy link
Member

left a comment

Looks good, thanks for doing this. Excited for this to land.

Maybe we should add a couple of the Angular @Input/@Output examples that were posted in the issues related to this PR as snapshot tests. But this looks good to me to merge now even without that.

@lydell

This comment has been minimized.

Copy link
Collaborator

commented Oct 5, 2018

@duailibe Sounds good 👍

@duailibe duailibe merged commit 303f344 into prettier:master Oct 8, 2018

0 of 4 checks passed

ci/circleci: checkout_code CircleCI is running your tests
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
deploy/netlify Deploy preview processing.
Details

@duailibe duailibe deleted the duailibe:decorators branch Oct 8, 2018

@ikatyang ikatyang added this to the 1.15 milestone Oct 25, 2018

@eranshmil eranshmil referenced this pull request Nov 9, 2018

Merged

style(prettier): fix format of project files #651

1 of 3 tasks complete

@lock lock bot locked as resolved and limited conversation to collaborators Jan 23, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.