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

[PropertyInfo] Add Component Documentation #5974

Merged
merged 5 commits into from
Aug 20, 2016
Merged

[PropertyInfo] Add Component Documentation #5974

merged 5 commits into from
Aug 20, 2016

Conversation

zanbaldwin
Copy link
Member

@zanbaldwin zanbaldwin commented Dec 7, 2015

Q A
Doc fix? no
New docs? yes (symfony/symfony#15858)
Applies to 2.8, 3.0
Fixed tickets N/A

This 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.

to/from objects and arrays, the PropertyInfo component works solely with class definitions to provide information
such as data type and visibility about properties within that class.

Similar to PropertyAccess, the PropertyInfo component blurs the lines between class properties such as ``$property``
Copy link
Member Author

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 idiom blurs the lines is not easily understood by non-native English speakers. Need to consider an alternative.

Copy link
Contributor

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"?

dunglas added a commit to symfony/symfony that referenced this pull request Dec 14, 2015
…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#5974

Unless 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:

```php
class X
{
    public $a;
    public $b;
    public function getA() {}
}
// Result: array('a', 'b');
```

```php
class Y
{
    public $A;
    public $b;
    public function setA() {}
}
// Result: array('A', 'b');
```

```php
class 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
such as data type and visibility about properties within that class.

Similar to PropertyAccess, the PropertyInfo component blurs the lines between class properties such as ``$property``
and properties defined via accessor and mutator methods such as ``getProperty()``, ``isProperty()``, ``setProperty()``,
Copy link
Member

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.

Copy link
Contributor

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
Copy link
Member

dunglas commented Dec 15, 2015

Good job! I left some minor comments but 👍

@zanbaldwin
Copy link
Member Author

Considering I failed English at school, the only language I can speak, thanks!
I'll try to finish this along with the edits within the next couple of days.

@zanbaldwin
Copy link
Member Author

Back in Europe, finally have internet access, any comments on this so far? /cc @dunglas.

@zanbaldwin zanbaldwin changed the title (WIP) [PropertyInfo] Add Component Documentation [PropertyInfo] Add Component Documentation Jan 22, 2016
@TomasVotruba
Copy link

This looks good to me. Anything more that needs to be done?

@zanbaldwin
Copy link
Member Author

The Creating Your Own Extractors section could possibly be expanded but I could never figure out how to word it :P

@TomasVotruba
Copy link

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.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

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
Copy link
Member

wouterj commented Jul 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
Copy link
Member Author

Hi guys, I've made the changes that were suggested. Does anyone have any other input?

@dunglas
Copy link
Member

dunglas commented Jul 21, 2016

Looks good to me. You did a very good job documenting this component, thank you!

@dunglas
Copy link
Member

dunglas commented Aug 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
Copy link
Member Author

Rebased, though you might want to check that I've solved all the conflicts correctly...

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>`
Copy link
Member

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)

Copy link
Contributor

@teohhanhui teohhanhui Aug 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* /components/property_access

@zanbaldwin
Copy link
Member Author

Got the build working, would you like the 5 commits squashed into 1?

@dunglas
Copy link
Member

dunglas commented Aug 16, 2016

Commits can be squashed by the merger.

Status: reviewed

@weaverryan weaverryan merged commit a0409d7 into symfony:2.8 Aug 20, 2016
weaverryan added a commit that referenced this pull request Aug 20, 2016
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/A

This 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 Tags
78ea4a5 Update PropertyAccess Component Path
7b786af Fix Incorrect Indent
96b139b Suggested Changes
cf7a0ea Add PropertyInfo Component Documentation
@weaverryan
Copy link
Member

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!

weaverryan added a commit that referenced this pull request Aug 21, 2016
This PR was merged into the 2.8 branch.

Discussion
----------

Tweaks to property info component

Just proofreading and making some tweaks to #5974

Commits
-------

c9ea21f bad link!
fcf5e69 fixing indentation
bf5188f final tweaks
7e296d8 [WIP] Tweaks to property info
@zanbaldwin zanbaldwin deleted the feature/component-property-info branch October 4, 2017 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants