Skip to content

Improvements to registering an extension (#2) #3236

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

Merged
merged 2 commits into from
Dec 23, 2013
Merged

Improvements to registering an extension (#2) #3236

merged 2 commits into from
Dec 23, 2013

Conversation

flip111
Copy link
Contributor

@flip111 flip111 commented Nov 28, 2013

Q A
Doc fix? yes
New docs? no
Applies to all
Fixed tickets #9644 (symfony) and #3231 (symfony-docs)
  1. The original text is good but the topic of that paragraph is not "extension conventions" but "registering an extension" (i.e. if you follow these conventions then register goes like so ..)
  2. Cleaned up different terminology "Conventions" v "Standards" -> just conventions now
  3. Move this part to a more logical place. First create then register, then modify for additional needs.

I don't know why a comment about Symfony 2.1 (versionadded:: 2.1) is removed in the new documentation (master). I copied from there.

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | all
| Fixed tickets | #9644 (symfony) and #3231 (symfony-docs)

1. The original text is good but the topic of that paragraph is not "extension conventions" but "registering an extension" (i.e. if you follow these conventions then register goes like so ..)

2. Cleaned up different terminology "Conventions" v "Standards" -> just conventions now

3. Move this part to a more logical place. First create then register, then modify for additional needs.

* The extension should provide an XSD schema.

Manually registering an Extension class
Copy link
Member

Choose a reason for hiding this comment

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

Manually Registering an Extension Class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It Looks Pretty, CamelCase, But No We Don't Write English Like This

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@wouterj
Copy link
Member

wouterj commented Nov 28, 2013

+1 this is indeed much better!

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

To manually register an extension class override the
:method:`Bundle::build() <Symfony\\Component\\HttpKernel\\Bundle\\Bundle::build>`
Copy link
Member

Choose a reason for hiding this comment

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

:method:`Symfony\\Component\\HttpKernel\\Bundle\\Bundle::build`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is outside of the scope of this PR (see improvements points 1,2 and 3). Please submit a new PR for this.

Copy link
Member

Choose a reason for hiding this comment

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

No it is not

@flip111
Copy link
Contributor Author

flip111 commented Nov 28, 2013

I'm not maintaining this PR anymore because of repo collab going out of scope.

@flip111 flip111 closed this Nov 28, 2013
@wouterj
Copy link
Member

wouterj commented Nov 28, 2013

Hi @flip111

I'm really sad you closing this pr... would be nice to get the changes in the docs.

From your comment, it looks like it was my fault? Could you please tell me what I did wrong? I and @xabbuh are reviewing almost all PRs, so it would be nice to learn how we can improve our reviewing comments.

Also, if you don't know how to fix the comments, don't heasitate (not sure if that's spelled correctly) asking us for help. We're always willing to help people contributing.

If yu really decide not to continue this, I'll create a new PR including your changes and a commit from me fixing the comments, so we can have this nice changes in the docs.

Thanks,
Wouter

@flip111
Copy link
Contributor Author

flip111 commented Nov 28, 2013

You closed the original PR because of "as there is nothing wrong with the docs and I can't see a way how we can rewrite it"

Then later when I submit this PR I get plenty of comments from you and xabbuh of all these little things being WRONG. Some of which I didn't even write in the first place. Many of which are valid points such as too long lines and title CamelCase (but the reason was not stated and I didn't know about the rules).

So all in all it seems to jump on the first chance to analyse, critique and dismiss it.

Also taken the hassle for submitting documentation when not being routined. I don't feel like doing additional work to add in changes that were already in the documentation before this PR. And then having the chance of it not being merged anyway (as there was zero comment on the things I actually did want to improve).

But since you were so nice to ask why I closed it, I will reopen it and do a commit on the changes you suggested.

@flip111 flip111 reopened this Nov 28, 2013
@wouterj
Copy link
Member

wouterj commented Nov 28, 2013

Hi, thanks for reopening!

I'll at some more explainations the next time, maybe my reviews become a little bit too robot-isch :)

The reason why we comment on small wrong things is because it's a lot easier to fix small things in the PR itself than after the merge. I spend a half year fixing many small things of PRs that were already merged, I want to avoid that in the feature.

And btw, I closed the precious issue as I thought anything was good, but as I already said in this PR, these changes are indeed better.

weaverryan added a commit that referenced this pull request Dec 23, 2013
Improvements to registering an extension (#2)
@weaverryan weaverryan merged commit 7cb9c23 into symfony:2.2 Dec 23, 2013
weaverryan added a commit that referenced this pull request Dec 23, 2013
@weaverryan
Copy link
Member

Thanks for re-opening this @flip111 - the section header and re-ordering really does make a lot more sense. I appreciate it!

Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants