Add Attributes style guide

[Crell]

This is based on what I've seen in the wild, with a healthy dose of my own judgement. It seems consistent with the rest of the document.

Resolves #8

[KorvinSzanto]

I'm not really a fan of including multiple attribute in a single #[...] especially when the attribute block isn't inline'd with a property.

For example, it feels unnecessarily hard to spot the Api at the end though syntax highlighting does help:

#[SomeLongAttributeWithABunchOfProperties(some: 'thing', i: 'will', definitely: 'see'), Api]
public function foo()
{
   ...
}

[Crell]

I don't like it either, but the language allows for it. A coding standard of "never use this language feature" seems... silly.

What I could see is maybe saying that attributes with arguments must always be in their own attribute block, but that's a fairly arbitrary rule I've not seen specified anywhere else. (I personally always put attributes in separate blocks, but I don't know if that's typical.)

[KorvinSzanto]

We definitely restrict language features with the goal of improving readability, a good example of that is compound namespaces under 3:

Compound namespaces with a depth of more than two MUST NOT be used

A good thing to keep in mind is that folks are welcome to say "We adhere to the Coding Style PER with the following changes:" and alter any hard requirements to fit their needs and so we can be a little more restrictive if readability benefits enough.

[Crell]

Sure, but we don't forbid compound namespaces entirely. That would be the equivalent here. I'm not comfortable just blanket disallowing a language feature, even if I never use it personally. (I never, ever, use compound namespaces, either.)

Would you be OK with a "only use if they're all argument-free" or something like that?

[samdark]

There's a SHOULD keyword for that purpose.

[KorvinSzanto]

Would you be OK with a "only use if they're all argument-free" or something like that?

That does help but I'm not convinced that Api is easily seen here even without any arguments:

#[SomeLongAttributeWithABunchOfProperties, Api]

I'd like to hear what other people think while we wait for the 2 week minimum on this PR.

[samdark]

I agree that soft-limiting it is a good idea.

[Crell]

I tried tweaking the wording a little.

Note that this still says MAY, only. So someone always and forever putting every attribute in its own block would be fully compliant.

[Crell]

Also rebased, so the section numbers all moved around.

[roxblnfk]

Hello. Could you add in the example the code block like this:

    public function __construct(
        #[Load(context: 'foo', bar: true)]
        private readonly FooService $fooService,

        #[LoadProxy(context: 'bar')]
        private readonly BarService $barService,
    ) {
    }

It can be useful to demonstrate the attributes usage in conjunction with the property promotion feature and the next rule:
Blank lines MAY be added to improve readability and to indicate related blocks of code except where explicitly forbidden.

[Crell]

Yeah, we should include a constructor, good call. For the blank lines, that's not in there now and I've heard differing opinions in the wild on it. I'm in favor of allowing them, but what does the rest of the WG think?

[samdark]

Allowing blank lines is fine.

[KorvinSzanto]

For the blank lines, that's not in there now and I've heard differing opinions in the wild on it. I'm in favor of allowing them, but what does the rest of the WG think?

Just leave it unstated and the "blank lines MAY be added to improve readability" rule will apply. Blank lines can be added most places to improve readability and I don't think this should be any different.

[alekitto]

I agree with @KorvinSzanto said here:

I'm not really a fan of including multiple attribute in a single #[...] especially when the attribute block isn't inline'd with a property.

I'm not a fan of it too, but if we want to allow multiple attributes in a single block, we can recommend to split in multiple lines like:

#[
  SomeLongAttributeWithABunchOfProperties(some: 'thing', i: 'will', definitely: 'see'),
  AnotherVeryLongAttributeHere(with: 'some', params: true),
  Api
]
public function foo() { ... }

[Crell]

But at that point wouldn't it be easier and just as effective to use separate blocks?

I don't know how to say that really long ones are a bad idea, but #[Foo, Bar] is fine. And I'm really not on board with saying that isn't OK.

[alekitto]

But at that point wouldn't it be easier and just as effective to use separate blocks?

That's why I prefer separate blocks

I don't know how to say that really long ones are a bad idea, but #[Foo, Bar] is fine.

The question is: where's the limit?
Foo and Bar are not really common in real-world applications and for clarity's sake I'd prefer to use separate blocks even in this case, rather than risk to miss an attribute just because the screen is not large enough to show it.

[roxblnfk]

The question is: where's the limit?

Limit is 120 chars per line :)

I don't think there is a need to set any limits on the number of attributes, their parameters, etc.
Allow the user to design attributes in different ways, and if the user wants to design the code beautifully, he will do it.

[alekitto]

Limit is 120 chars per line :)

Fair enough.

The thing I want to note is that

#[SomeLongAttributeWithABunchOfProperties(some: 'thing', i: 'will', definitely: 'see'), Api]

is 92 chars long (100 chars with 8 spaces of indentation), so it is ok with the limit, but still the Api part is completely transparent at first sight.

That said, if we want to allow multiple attributes as longs as they don't break the chars per line rule, it's ok for me.

[KorvinSzanto]

Limit is 120 chars per line :)

That's a soft limit and not a requirement. If we wanted that cemented for attributes we'd have to explicitly state so. I don't really think we want to limit the line length of attributes more than what we already have since attributes are likely to see deep nesting and to use constants with long names.

Allow the user to design attributes in different ways, and if the user wants to design the code beautifully, he will do it.

This is the antithesis of a code style guideline, the point of having a guideline is to ensure code looks the same across authors and across projects. There are definitely places where we can take this approach - and this might be one of them - but we should err on the side of defining things more and leaving things undefined less.

---

Responding to @Crell:

I don't know how to say that really long ones are a bad idea, but #[Foo, Bar] is fine. And I'm really not on board with saying that isn't OK.

I'm not convinced that allowing #[Foo, Bar] adds value. To me it makes more sense to disallow it and have folks who'd like to use compound attributes specifically allow it like we do with folks who would prefer tabs over spaces.

If we could just think of a way to articulate the thinking of "Long complex = bad, short simple = good" that doesn't leave things entirely subjective like "reasonably short" does it'd be easier to include. That said I think the rest of this looks good to me and I'd be willing to merge it if we did the following:

1. Remove the bit that allows compound attributes like #[Foo, Bar] as well as the examples that show compound attributes
2. Leave compound attributes unspecified
3. Open an issue to continue discussion so we can decide how to proceed

[KorvinSzanto]

See #26 (comment)

[Crell]

We can keep discussing in another thread, but I want to reiterate that "the language explicitly supports syntax X, but we're forbidding you from using it" is not a position I can support, even if I personally have never used compound attributes. So it's all a matter of finding a better/more precise way of saying "reasonably short" (which I agree is suboptimal). I also don't think "well people can just violate the spec if they really want to" is a good position to take. That undermines the entire idea of having a unified style guide. (Yes, we cannot enforce it, but we want it to be something that the majority of the ecosystem can use exactly as-is or it's ineffective.)

[KorvinSzanto]

To be clear what I'm asking for here is to leave this unspecified for now, not to disallow it.

[Crell]

So something like this?

[KorvinSzanto]

No not quite, your revision leaves parts discussing compound attributes and includes examples of them. What I'm asking for is to remove the parts that refer to compound attributes and leave compound attributes entirely unspecified for now. I can go through and make a suggested change at some point this week while I'm at a conference if you're still unsure of what that'd look like.

If we're not able to extract that contentious bit from this PR for now this PR will have to hang out until all parts are agreeable which is totally fine.

[Crell]

We cannot reasonably pretend that they don't exist. I'd rather acknowledge their existence and put reasonable bounds on them (as here) and punt on the "when you should use them" question than punt on their existence entirely.

[Crell]

Can we get this resolved? I don't really see much alternative to what is listed here now. Ignoring the existence of compound attributes is not viable, and forbidding the use of a valid language feature is also not viable. That leaves us with some variation on "use sparingly" as the only option.

[mathroc]

Ignoring the existence of compound attributes is not viable, and forbidding the use of a valid language feature is also not viable

What makes this a feature that can't be ignored? For example, having a comma after the last item of an array is a valid construct even if it's on a single line. What makes those different? It's not like there's no alternative to compound attributes, right?

[KorvinSzanto]

I haven't heard any other WG members concerned about this so I'll merge as is so we can move forward. If anyone else wants to discuss this please open an issue.