Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
[PropertyInfo] Add Component Documentation#5974
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[PropertyInfo] Add Component Documentation#5974
Uh oh!
There was an error while loading.Please reload this page.
Conversation
components/property_info.rst Outdated
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.
As pointed out by a friend in the Philippines, the idiomblurs the lines is not easily understood by non-native English speakers. Need to consider an alternative.
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 about "works with both"?
…actor (zanderbaldwin)This PR was squashed before being merged into the 2.8 branch (closes#16911).Discussion----------[PropertyInfo] Update List Information from ReflectionExtractor| Q | A| ------------- | ---| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#16889| License | MIT| Doc PR |symfony/symfony-docs#5974Unless a property with the same casing exists, lowercase the first letter of a property name extracted from a method. From what I understand, we don't actually need to support `snake_case` at all in the PropertyInfo component.I cannot think of any use-case where PropertyAccess would be used to get/set a property value *before* using PropertyInfo to find out information about the property and the values it supports.Since PropertyInfo supports the discovery of property names, which can then be used as identifiers in PropertyAccess, there should be no need to support a naming strategy to map property identifiers from one naming convention to another.---Running `$reflectionExtractor->getProperties($class)` with the following classes:```phpclass X{ public $a; public $b; public function getA() {}}// Result: array('a', 'b');``````phpclass Y{ public $A; public $b; public function setA() {}}// Result: array('A', 'b');``````phpclass Y{ public $username; protected $emailAddress; public $password; public function getEmailAddress() {} public function isActive() {}}// Result: array('username', 'emailAddress', 'password', 'active');```Commits-------b2da76c [PropertyInfo] Update List Information from ReflectionExtractor
components/property_info.rst Outdated
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.
It is also able to leverage adders and removers.
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 list of example method names is not illustrative. I think some actual examples would be better.
dunglas commentedDec 15, 2015
Good job! I left some minor comments but 👍 |
zanbaldwin commentedDec 15, 2015
Considering I failed English at school, the only language I can speak, thanks! |
zanbaldwin commentedJan 12, 2016
Back in Europe, finally have internet access, any comments on this so far? /cc@dunglas. |
TomasVotruba commentedMar 6, 2016
This looks good to me. Anything more that needs to be done? |
zanbaldwin commentedMar 6, 2016
TheCreating Your Own Extractors section could possibly be expanded but I could never figure out how to word it :P |
TomasVotruba commentedMar 6, 2016
I think that might be next, advanced article. I'd add just this simple "How to use this" version. It's really missed at the moment. |
components/property_info.rst Outdated
| The PropertyInfo component provides the functionality to get information about class properties using metadata of | ||
| popular sources. | ||
| Whilst the :doc:`PropertyAccess component</components/property_access/introduction>` allows you to read and write values |
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 see multiple occurrences of "whilst".
Seehttp://dictionary.cambridge.org/grammar/british-grammar/while-and-whilst
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'll edit that now.
wouterj commentedJul 8, 2016
Hi @zanderbaldwin! I'm sorry that it took so long before one of us reviewed this PR. I think it's a nice start. Can you please fix the comments I added to this PR and make sure lines are wrapped after the first word that crosses the 72th character (they are a bit too long atm)? Thanks already! If you don't have time, just comment and we'll continue this PR. status: needs work |
zanbaldwin commentedJul 20, 2016
Hi guys, I've made the changes that were suggested. Does anyone have any other input? |
dunglas commentedJul 21, 2016
Looks good to me. You did a very good job documenting this component, thank you! |
dunglas commentedAug 16, 2016
@zanderbaldwin can you rebase your PR? @xabbuh@wouterj@weaverryan can this PR be merged? It would be nice to have the doc for this component online. |
zanbaldwin commentedAug 16, 2016
Rebased, though you might want to check that I've solved all the conflicts correctly... |
components/property_info.rst Outdated
| The PropertyInfo component provides the functionality to get information | ||
| about class properties using metadata of popular sources. | ||
| While the:doc:`PropertyAccess component</components/property_access/introduction>` |
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 path should be updated to/components/property_info (it's why Travis fails)
teohhanhuiAug 16, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
*/components/property_access
zanbaldwin commentedAug 16, 2016
Got the build working, would you like the 5 commits squashed into 1? |
dunglas commentedAug 16, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Commits can be squashed by the merger. Status: reviewed |
This PR was merged into the 2.8 branch.Discussion----------[PropertyInfo] Add Component Documentation| Q | A| ------------- | ---| Doc fix? | no| New docs? | yes (symfony/symfony#15858)| Applies to | `2.8`, `3.0`| Fixed tickets | N/AThis is still a *work in progress*, but I'd like feedback from anyone regarding structure, grammar and any other improvements while I continue. After speaking with@dunglas, I'm creating a new PR due our branches differing by about 400 commits.Commits-------a0409d7 Update Reference to Service Container Tags78ea4a5 Update PropertyAccess Component Path7b786af Fix Incorrect Indent96b139b Suggested Changescf7a0ea Add PropertyInfo Component Documentation
weaverryan commentedAug 20, 2016
Merged! This is great! I was very much looking forward to the docs for this, but bootstrapping the docs for a new component is a lot of work :). So, thank you! I opened#6896 with some proofreading tweaks that I'm making. I'll finish it shortly, but I just got interrupted :). Cheers! |
Uh oh!
There was an error while loading.Please reload this page.
2.8,3.0This is still awork in progress, but I'd like feedback from anyone regarding structure, grammar and any other improvements while I continue. After speaking with@dunglas, I'm creating a new PR due our branches differing by about 400 commits.