-
Notifications
You must be signed in to change notification settings - Fork 2
Add attribute template #3
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
Conversation
e54ca1c to
282b881
Compare
282b881 to
fcc04c8
Compare
jrfnl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just left some small suggestions.
Template.php
Outdated
| namespace Fig\Attributes; | ||
|
|
||
| /** | ||
| * One-Line summary of what the attribute does |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * One-Line summary of what the attribute does | |
| * One-Line summary of what the attribute does. |
|
The last comment reminds me: It may not be super relevant, but should we allow using PHPStan/PSalm extended annotations, like |
Template.php
Outdated
| * @param boolean $expose | ||
| * Whether to expose this as an example or not. | ||
| * @param int[] $luckyNumbers | ||
| * The lucky numbers of this week |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * The lucky numbers of this week | |
| * The lucky numbers of this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This (as well.as the first line of the class docblock) Is considered a Subject line. It should not contain punctuation.
It's not a sentence but a short summary like a ToC entry.
I like to think of it as a commit.message for the class/function/parameter. The first line in imperative mood extending the sentence "When this class/function/parameter is used it will..."
Applying the Beams-Rules will lead to clear and concise informations. I am willing to increase the 50 to 76 on the second rule, but that should be it.
If one can't summarize the class/function/parameter functionality in 76 characters, that usially is a hint that it does too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have never heard of such a rule (and the $expense property has a period, so it's inconsistent right now either way). And IME, I disagree entirely. Very often there is context needed for a parameter that makes more sense on the parameter than in a description block. Most of my parameters have no description (because they're self-evident from the name and method), some have a short description, and some have lengthy descriptions for context. I don't see a reason to forbid the latter, for which complete sentences with proper punctuation is appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The summary of a docblock is described in the draft of psr-5. I updated that recently. In psr-5 we state that the dot at the end of the summary is optional, but recommended.
I think we didn't include anything about the description of a parameter. Although tags can have a longer description than just a short sentence, there is no definition on how such a description should be formatted.
I will put this on my list to address in psr-5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I stumbled upon this similarity of a function/parameter/class description to a commit message just recently but it made perfect sense to me.
"when this parameter is used it will:" Merge two or more arrays1, Create an array by using one array for keys and another for its values2, Tokenize a string3
These are just some random examples from the PHP-Docs that seem to follow the same idea of an imperative sentence. BTW all of them without a dot at the end.
Yes! Admittedly in the parameter descriptions of all those examples that is no longer followed and none of the titles are in imperative mood any more.
Whether there is a dot at the end or not is not a hill I will die upon. But for me those sentences are the headline of the following more thorough description. The TOC entry. And in headlines we rarely find periods at the end of the sentence4.
On the other hand the Oracle Docs on how to write DocComments for the JavaDoc tool state that "The first sentence of each doc comment should be a summary sentence, containing a concise but complete description of the API item". They then highlight that it needs a dot at the end as that is a technical requirement for the tool. They do though not mention anything about Imperative mood or even a blank line between the summary/Headline and the remainder of the documentation text.
To me a clear structure like
# Summarize the class functionality
Describe the class functionality in more detail.
even in several paragraphs.
## Summarize the first method
Describe the first method in more detail.
### Summarize first parameter to method without details
### Summarize second parameter to method
Describe parameter in more detail.
...
makes the most sense.
But to make things short (after this lengthy excursion) - If that is covered in PER-5 we should just reference that.
I'll see how I can modify that...
Footnotes
Template.php
Outdated
| * Also, you SHOULD describe what the different functions do. | ||
| * | ||
| * @param string $name | ||
| * The name of this template |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * The name of this template | |
| * The name of this template. |
Template.php
Outdated
| * You MUST describe parameters to your Attribute extensively! You SHOULD use | ||
| * the well-known Annotations for that. | ||
| * | ||
| * Also, you SHOULD describe what the different functions do. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * Also, you SHOULD describe what the different functions do. | |
| * Also, you SHOULD describe what the different parameters do. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant "functions". People should des ribe what the functions do, not only the parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What functions? Do you mean methods should have docblocks? If so, say that directly.
|
I like the idea of a template, but using the name Template makes it impossible to introduce an attribute in the future that reflects the usage of a template attribute. Maybe we should make this template not a php file but a markdown file describing the template? |
|
As long as it's not in the |
When we need more complex structures than simple lists of scalar values, we can always introduce a value object right? |
|
@heiglandreas Are you going to finish updating here? I want to restructure the Pure attribute into this format and see how it works. |
This movedthe template-file from a PHP-file to a markdown-file to not block the `template` name for an attribute. It also enhances the content slightly by referencing PSR-5, using dots at the end of all the subject lines, changing "function" to "method" in one place and extend the description of a parameter to show that multi-line is possible there as well.
|
I moved the template into an MD file. I think it indeed makes sense to have the file not being a PHP-file to avoid confusion. I also added dots to the subject lines. It's not a hill I'll die upon. Looking forward to your feedback |
jaapio
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be good for a first start. Let's see how this turns out in practice.
This adds a template for an attribute definition that can help people get started when they want to create a PR for a new attribute.
It should contain all possible options that help someone document the new attribute as good as possible. This will for sure be a living document as we won't think of all possibilities and options right from the start.