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

Never inline decorators, unless they're lone parameter decorators #4830

Merged
merged 3 commits into from Jul 20, 2018

Conversation

Projects
None yet
10 participants
@suchipi
Copy link
Member

suchipi commented Jul 10, 2018

Fixes #2613.

cc @vjeux who might have more context than me on this.

constructor(@ILifecycleService lifecycleService) {}
constructor(
@ILifecycleService
lifecycleService

This comment has been minimized.

Copy link
@suchipi

suchipi Jul 10, 2018

Author Member

What does this syntax mean?

This comment has been minimized.

Copy link
@j-f1

j-f1 Jul 10, 2018

Member

Here’s what TS has to say:

The expression for the parameter decorator will be called as a function at runtime, with the following three arguments:

  1. Either the constructor function of the class for a static member, or the prototype of the class for an instance member.
  2. The name of the member.
  3. The ordinal index of the parameter in the function’s parameter list.

This comment has been minimized.

Copy link
@suchipi

suchipi Jul 10, 2018

Author Member

Interesting. Their example makes it a bit clearer:

class Greeter {
    greeting: string;

    constructor(message: string) {
        this.greeting = message;
    }

    @validate
    greet(@required name: string) {
        return "Hello " + name + ", " + this.greeting;
    }
}

We should keep these inline.

@observable amount: number = 1;
@observable
price: number = 0;
@observable

This comment has been minimized.

Copy link
@suchipi

suchipi Jul 10, 2018

Author Member

It felt a bit odd to not have an empty line between these, but since we preserve user-entered empty lines, I think it's fine (see also the type orm snapshot)

? parent === firstNonConditionalParent
? group(doc)
: doc
? parent === firstNonConditionalParent ? group(doc) : doc

This comment has been minimized.

Copy link
@suchipi

suchipi Jul 10, 2018

Author Member

My bad, my editor formatted with an older version of prettier

: hasComment && !isOpeningFragment
? " "
: "",
: hasComment && !isOpeningFragment ? " " : "",

This comment has been minimized.

Copy link
@suchipi

suchipi Jul 10, 2018

Author Member

here too

@vjeux

This comment has been minimized.

Copy link
Collaborator

vjeux commented Jul 10, 2018

Originally I put all of them in their own line but there were a lot of people using mobx which complained. Apparently it’s the convention over there to inline them.

I have no opinions myself.

@suchipi suchipi added the status:wip label Jul 10, 2018

@egeozcan

This comment has been minimized.

Copy link

egeozcan commented Jul 11, 2018

@vjeux mobx is the only library which inlines them as a convention. even in the spec itself, they are on their own line. I couldn't find any language which has something similar to decorators and inlined them by convention. See the last comments on #2613

@kamilmysliwiec

This comment has been minimized.

Copy link

kamilmysliwiec commented Jul 13, 2018

Fully agree. Inline decorators look awful and it's quite a standard to keep them in the separated lines. TypeOrm entities, as well as NestJS/Angular stuff, looks really bad once they're inlined. Actually, I went here because I was pretty sure that this issue is finally sorted out.

@suchipi

This comment has been minimized.

Copy link
Member Author

suchipi commented Jul 17, 2018

I'm hoping to get this into the 1.14 release, but I won't have time to work on this until Thursday evening. I'll add it to the milestone for now, but other maintainers, remove it if you need to (or finish the work for me 😝).

@suchipi suchipi added this to the 1.14 milestone Jul 17, 2018

@suchipi suchipi force-pushed the suchipi:dont_inline_decorators branch from 90c7b9e to c458d34 Jul 20, 2018

@suchipi suchipi removed the status:wip label Jul 20, 2018

@suchipi suchipi changed the title Never inline decorators Never inline decorators, unless they're parameter decorators Jul 20, 2018

@suchipi

This comment has been minimized.

Copy link
Member Author

suchipi commented Jul 20, 2018

Ok, this is ready for review/merge

@suchipi suchipi changed the title Never inline decorators, unless they're parameter decorators Never inline decorators, unless they're lone parameter decorators Jul 20, 2018

if (
!(
parent[propertyName] &&
Object.prototype.toString.call(parent[propertyName]) === "[object Array]"

This comment has been minimized.

Copy link
@duailibe

duailibe Jul 20, 2018

Member

You can use Array.isArray (we use it in several places)

This comment has been minimized.

Copy link
@suchipi

suchipi Jul 20, 2018

Author Member

Oh nice! I'll change it.

This comment has been minimized.

Copy link
@j-f1

j-f1 Jul 20, 2018

Member

Array.isArray isn’t supported on Node 4; do we polyfill?

This comment has been minimized.

Copy link
@duailibe

duailibe Jul 20, 2018

Member

@j-f1 it is supported. On node 4, it doesn't work with Proxys or subclasses

@suchipi suchipi merged commit 3bfaf66 into prettier:master Jul 20, 2018

9 of 10 checks passed

codecov/project 96.45% (-0.02%) compared to 1fe0b01
Details
ci/circleci: build_prod Your tests passed on CircleCI!
Details
ci/circleci: checkout_code Your tests passed on CircleCI!
Details
ci/circleci: test_prod_node4 Your tests passed on CircleCI!
Details
ci/circleci: test_prod_node9 Your tests passed on CircleCI!
Details
ci/circleci: test_prod_standalone Your tests passed on CircleCI!
Details
codecov/patch 88.88% of diff hit (target 80%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@Hoten

This comment has been minimized.

Copy link

Hoten commented Jul 23, 2018

I'd like to thank the project maintainers for listening to the community on this issue. Thank you for making a great tool!

@devuxer

This comment has been minimized.

Copy link

devuxer commented Aug 17, 2018

I just noticed this change when formatting a file in a mobx project.

I get the motivation for it, so I don't disagree with putting decorators on a separate line, but I think it would be more readable to put an extra line break between decorated properties.

Harder to read

@observable
public name = "";
@observable
public location = "";
@observable
public nationId = "";

Easier to read

@observable
public name = "";

@observable
public location = "";

@observable
public nationId = "";

Agree/disagree? I can make this a new issue if people agree.

@lydell

This comment has been minimized.

Copy link
Collaborator

lydell commented Aug 17, 2018

So far, Prettier has never inserted blank lines into your code, as far as I know (only removed/collapsed blank lines). So this would be the first case where it inserts them. Might be good, might be confusing. Not sure. Anyway, check out the discussion in #4924 first.

@devuxer

This comment has been minimized.

Copy link

devuxer commented Aug 17, 2018

@lydell,

Very interesting. I guess the premise was incorrect that mobx was the only thing using inline decorators, so this is really quite a tough one to solve. I put a suggestion in #4924 as well.

@Rob1kelly15

This comment has been minimized.

Copy link

Rob1kelly15 commented Aug 24, 2018

Is there a way to configure this to turn off? We work in Angular 6, and when it comes to our input and output decorators, we want them inline, but this prevents us from doing so, since we have it to auto format on save

@j-f1

This comment has been minimized.

Copy link
Member

j-f1 commented Aug 24, 2018

Please discuss this over in #4924.

@prettier prettier locked and limited conversation to collaborators Aug 24, 2018

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.