Skip to content

Fix use mistakes #4120

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

Closed
wants to merge 2 commits into from
Closed

Fix use mistakes #4120

wants to merge 2 commits into from

Conversation

mbutkereit
Copy link

Q A
Doc fix? yes
New docs? no
Applies to all

On http://symfony.com/doc/current/components/config/resources.html at "new YamlUserLoader", we have also a missing "use" statement. But i'm not sure if we need there one.

@wouterj
Copy link
Member

wouterj commented Aug 13, 2014

Wow, you've gone use statement hunting! Nice job of fixing some of the examples!

Some tips for the next time you create PRs (you don't have to edit this PR, it's just in case you want to contribute more):

  • It's better to create a new branch for each PR. This means you can submit multiple PRs at the same time (a PR is related to a whole branch, not to a specific changeset).
  • In the documentation, we have branches for each Symfony version. Currently, we are maintaining 2.3, 2.4, 2.5 and master versions of the docs. We merge them up from the oldest (2.3) to the newest (master), so all fixes in 2.3 will also be applied in 2.4, 2.5 and master. So if you fix something, you should always base your PR on 2.3. And only if you document new features, you should base your PR on one of the other branches.

To comment on your question:

On http://symfony.com/doc/current/components/config/resources.html at "new YamlUserLoader", we have also a missing "use" statement. But i'm not sure if we need there one.

In the definition of the YamlUserLoader class, we don't use a namespace, so I think we're good.

@@ -104,6 +104,8 @@ Asset path generation is handled internally by packages. The component provides

You can also use multiple packages::

use Symfony\Component\Templating\Asset\PathPackage;
Copy link
Member

Choose a reason for hiding this comment

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

If we add a use statement for the PathPackage class, we should also add one for the AssetsHelper.

@mbutkereit @wouterj What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I disagree, that is already covered by the // ...

@weaverryan
Copy link
Member

Thanks for making these changes @mbutkereit - you rock!

weaverryan added a commit that referenced this pull request Aug 16, 2014
This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes #4120).

Discussion
----------

Fix use mistakes

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     |  no
| Applies to    | all

On http://symfony.com/doc/current/components/config/resources.html at "new YamlUserLoader", we have also a missing "use" statement. But i'm not sure if we need there one.

Commits
-------

7016c82 Fixing a mistake
4353d1f Fix use mistakes.
@wouterj wouterj closed this Aug 16, 2014
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.

4 participants