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 multiple property and argument decorators on same line as argument when possible (Babylon, TypeScript) #1974

Closed
mafredri opened this issue Jun 5, 2017 · 22 comments
Labels
lang:javascript Issues affecting JS lang:typescript Issues affecting TypeScript-specific constructs (not general JS issues) locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.

Comments

@mafredri
Copy link

mafredri commented Jun 5, 2017

Prettier 1.10.2
Playground link

--parser typescript

Input:

export class CoreModule {
  @Input() @Output() prop: string;

  constructor(@Optional() @SkipSelf() parentModule: CoreModule) {}
}

Output:

export class CoreModule {
  @Input()
  @Output()
  prop: string;

  constructor(
    @Optional()
    @SkipSelf()
    parentModule: CoreModule
  ) {}
}

Expected output:

No difference between input and output.

If we did the same thing with only one decorator, the result would be:

export class CoreModule {
  @Input() prop: string;

  constructor(@Optional() parentModule: CoreModule) {}
}

In this sense I don't think breaking on multiple decorators adds any value, instead adds more visual noise and makes it harder to spot arguments in e.g. the constructor.

@azz azz added the status:needs discussion Issues needing discussion and a decision to be made before action can be taken label Jun 5, 2017
@azz
Copy link
Member

azz commented Jun 5, 2017

Personally I prefer the current behavior but I'm interested to hear other opinions.

@mafredri
Copy link
Author

mafredri commented Jun 5, 2017

Personally I prefer the current behavior but I'm interested to hear other opinions.

Ok, that's fair. I was surprised by the behavior, which is why I opened the issue. Especially with multiple function arguments I thought it decreased visibility, e.g. visually parsing actual arguments.

I'm fine with the current behavior though, I can get used to it, so leave this open as long as you like 👍.

@dalemanthei
Copy link

dalemanthei commented Jun 29, 2017

The angular style guide shows a preference for putting the annotations like @Input() and @Output() on the same lines. It is a little nicer when you want to quickly find the inputs and outputs (ag anyone?)

For those reasons I would like the code that follows to stay on same line as the annotations.

I previewed Prettier to my team and after inspecting what it did with our code, all 3 of us voted to adopt.

@itsMapleLeaf
Copy link

itsMapleLeaf commented Jun 30, 2017

I'd prefer for there to be an option, at least. Decorators tend to read as keywords in usage, so it'd make sense for them to go on the same line, alongside async, public, and so on. Putting them all on an extra line tends to waste a lot of vertical space.

A comparison:

  @action
  setView(view: AppView) {
    this.view = view
  }

  @action
  setLoginStatus(status: string) {
    this.loginStatus = status
  }

  @action
  setIdentity(identity: string) {
    this.chat.identity = identity
  }
  @action setView(view: AppView) {
    this.view = view
  }

  @action setLoginStatus(status: string) {
    this.loginStatus = status
  }

  @action setIdentity(identity: string) {
    this.chat.identity = identity
  }

I think it's a fair compromise to go multi-line when there's more than one. Or to simply do so once the line hits the line limit.

@azz azz added lang:javascript Issues affecting JS lang:typescript Issues affecting TypeScript-specific constructs (not general JS issues) labels Oct 4, 2017
@kedrzu
Copy link

kedrzu commented Nov 2, 2017

Inlining decorators make document look pretty incosistent, when you mix props/methods with and without decorators:

class Foobar {

    @action public bar() {
    };

    public boom() {
    }
}

The same applies when putting varying number of decorators.

From my point of view aligning prop/method names evenly is far more readable than having this alignment being broken with some inlined decorators. It's quite hard to scan the file if you mix multiple conventions depending on number of decorators, width of line and such.

Here is another (real) example:

export default class SaleOrderDetailsView extends Vue {
    @data() protected orderData: any[] = [];

    @mounted
    private initialize() {
    
    // .. rest of file

This is how prettier formatted code - property is aligned differently than method. It is overall not easy to read, as your eyes need to jump vertically and horizontally to find a name you look for.

In my honest opinion best way is to provide an option to choose from. I see that this issue was raised multiple times and people are quite divided about it. And of course that is perfectly OK, as programming style is not about using some best and opinionated methodologies, but about being consistent.

@tmueller
Copy link

Maybe it would be best to have an option for keeping decorators on the same line or always have them on a preceding line. You find many examples on github for both styles.

@dalemanthei
Copy link

I like the idea of an option for 1) always on own line or 2) start on same line but respect the line length option.

@jtomaszewski
Copy link

IMHO this should be controlled by an option:

  1. you always have decorators on a preceding line, 1 per line
export class MyComponent {
  @Input()
  public input: string;

  @Input()
  @Output()
  public payload: number = 123;

  @Input()
  @DecoratorWithVeeeeeryLongName()
  @DecoratorWithSuuuuuupeeeeerrrrVeeeeeryLongName()
  public anotherPayloadWithLongName: number = 456;

  constructor(
    elementRef: ElementRef,
    @Optional() 
    @Self()
    ngControl: NgControl
  ) {}
}
  1. you always have decorators on one preceding line (unless it extends the line width - then move it to a next line)
export class MyComponent {
  @Input()
  public input: string;

  @Input() @Output()
  public payload: number = 123;

  @Input() @DecoratorWithVeryLongName()
  @DecoratorWithSuuuuuupeeeeerrrrVeeeeeryLongName()
  public anotherPayloadWithLongName: number = 456;

  constructor(
    elementRef: ElementRef,
    @Optional() @Self()
    ngControl: NgControl
  ) {}
}
  1. you always have decorators on the same line (unless it extends the line width - then move the variable definition to the next line)
export class MyComponent {
  @Input() public input: string;

  @Input() @Output() public payload: number = 123;

  @Input() @DecoratorWithVeryLongName()
  @DecoratorWithSuuuuuupeeeeerrrrVeeeeeryLongName()
  public anotherPayloadWithLongName: number = 456;

  constructor(
    elementRef: ElementRef,
    @Optional() @Self() ngControl: NgControl
  ) {}
}

The current (as of 1.10.2) behaviour is neither of it. It keeps the decorator in one line if possible, and if not, then keeps all the decorators in separate lines (which is a bit weird IMO). You can see it below:

export class MyComponent {
  @Input() public input: string;

  @Input()
  @Output()
  public payload: number = 123;

  @Input()
  @DecoratorWithVeryLongName()
  @DecoratorWithSuuuuuupeeeeerrrrVeeeeeryLongName()
  public anotherPayloadWithLongName: number = 456;

  constructor(
    elementRef: ElementRef,
    @Optional()
    @Self()
    ngControl: NgControl,
  ) {}
}

Myself, I'd use option number 3 . This keeps stuff like @Optional() @Self() ngControl or @Input() @WithDefaultValue('placeholder') label in one line, so I can see it right away when f.e. I'm searching my file for all input variable occurrences.

@kedrzu
Copy link

kedrzu commented Feb 20, 2018

Also, for now Prettier is always breaking line if you provide any parameter to your decorator:

class Foobar {
    @Party() public foo: string; 

    @Party({ alcohol: true })
    public boom: string;
}

This also seems quite inconsistent.

@andrew-luminal
Copy link

👍 to this, prettier has made some of my stores with decorators very hard to read.

@setter @observable list;
@observable item;
@setter observable list2;

becomes

@setter
@observable
list;
@observable item
@setter
@observable
list2;

@antoniusostermann
Copy link

Having the same problem. If you use InversifyJS (http://inversify.io/), prettier makes it harder for you to recognize constructor arguments assigned with multiple decorators (e. g. @optional @inject() a, ...)

@kedrzu
Copy link

kedrzu commented Mar 26, 2018

@andrew-luminal @antoniusostermann
You see, this is very subjective: for example, I would rather have decorators in separate lines. However, I may be quite skewed, as I have C# background :)
Nevertheless, this is why I would be very pleased to see more freedom of choice here.

@babeal
Copy link

babeal commented Mar 27, 2018

I disagree, I do not want decorators on the same line for properties thought I would like to keep them on the same line for constructor parameters. Definitely not for class properties. I really dislike that convention and hope that you have a setting for this in the future

@gregwym
Copy link

gregwym commented Apr 12, 2018

When class members are all decorated with different decorators, putting them inline makes it really hard to read what the class member really is.
In our case, TypeORM have uneven length decorators for entity columns, inline decorator makes it really hard to recognize all the column names.

@Entity()
export class User extends BaseEntity {
  @PrimaryGeneratedColumn('uuid') id: string;

  @Column() firstName: string;

  @Column() lastName: string;

  @Column({ default: '' })
  gender: string;

  @CreateDateColumn() createdAt: Date;

  @UpdateDateColumn() updatedAt: Date;
}

@ychaikin
Copy link

ychaikin commented Apr 19, 2018

My 2 cents:

Create an option called something like honor-decorator-newline: boolean

If true, formatting would honor the newline character. If false, it would continue formatting it the way prettier is formatting things now.

Advantages:

  • Allows current source code already using current prettier to stay as is
  • Allows people who prefer annotations for class/property members to have the annotation on its own line with this option.
  • With this option set to true, allows user to format the function argument annotations (with regard to setting it on its own line) manually. If line is too long, regular wrapping rules will apply anyway.

Annotations are not unique to Typescript and there are a LOT of code out there with annotations, especially on class members, being formatted on its own line. Point is, there is a very strong case for allowing a very strong custom/preference already in existence to continue.

I believe Google style for Java code does just that. Allows an option to honor user newline characters in certain circumstances. (For example, in IntelliJ, there is an option called Keep when reformatting and Line breaks is one of the options for that.)

I can provide markdown for all the cases I just mentioned if anyone is interested or unclear.

@shimarulin
Copy link

For keep line breaks in now you can use single-line comments, see playground. Of course this is not a solution that is expected, but better than nothing. Previous proposal looks reasonable, it would be nice to have such an option.

@devomacdee
Copy link

Has this been updated? I am still having this issue with the decorators always being placed on a new line. Is there now an option to disable this?

@lydell
Copy link
Member

lydell commented Aug 15, 2018

@devomacdee See #4924 (comment)

@btmurrell
Copy link

btmurrell commented Sep 19, 2018

damn, people, all this discussion on preferences dictates that this should be a preference. Angular style guide says one line. Those using Angular might want to break from that recommendation. Bottom line is make it configurable so we are not forced to take the provider's style. This issue has been open way too long. Why not just meet the needs of the community and make it configurable? Every time I get the nice benefits of prettier, I have to run a search/replace to undo the decorator formatting that does not comply with our team's style. :grrr:

@lydell
Copy link
Member

lydell commented Sep 29, 2018

Current plan: #4924 (comment)

@birkir
Copy link

birkir commented Jan 26, 2019

Ugh, all issues and PR's related to class function argument decorators have been closed.

It is still a problem: link to playground

@shimarulin Thanks for the pro-tip, that works for me until this gets "fixed", and I say fixed because this is a massive readability issue as typescript projects are moving more and more into decorators. (see TypeORM or TypeGraphQL)

@lydell
Copy link
Member

lydell commented Jan 27, 2019

I’m going to close this since the OP’s example has been fixed:

Prettier 1.16.1
Playground link

--parser typescript

Input:

export class CoreModule {
  @Input() @Output() prop: string;

  constructor(@Optional() @SkipSelf() parentModule: CoreModule) {}
}

export class CoreModule {
  @Input()
  @Output()
  prop: string;

  constructor(
    @Optional()
    @SkipSelf()
    parentModule: CoreModule
  ) {}
}

Output:

export class CoreModule {
  @Input() @Output() prop: string;

  constructor(@Optional() @SkipSelf() parentModule: CoreModule) {}
}

export class CoreModule {
  @Input()
  @Output()
  prop: string;

  constructor(
    @Optional()
    @SkipSelf()
    parentModule: CoreModule
  ) {}
}

@birkir Feel free to open a new issue about your problem!

@lydell lydell closed this as completed Jan 27, 2019
@lydell lydell removed the status:needs discussion Issues needing discussion and a decision to be made before action can be taken label Jan 27, 2019
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Apr 27, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Apr 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lang:javascript Issues affecting JS lang:typescript Issues affecting TypeScript-specific constructs (not general JS issues) locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

No branches or pull requests