Skip to content

Removed literals for bundle names #4865

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 1 commit into from
Jan 25, 2015

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Jan 16, 2015

Note: Most of it was automated and I didn't review it closely yet (since I didn't know the general opinion on this topic), so there can be some faulty changes.

I always like to use as less literals as possible, as I think it makes sentences harder to understand. That's why I strongly believe that literals should exclusively be used for code. As a bundle name is not code, I propose to remove all literals around bundle names.

Q A
Doc fix? yes
New docs? nope
Applies to all
Fixed tickets -

@stof
Copy link
Member

stof commented Jan 16, 2015

I tend to agree with you

@@ -45,7 +45,7 @@ and put things there:

.. tip::

The recommended approach of using the ``AppBundle`` directory is for
The recommended approach of using the AppBundle directory is for
Copy link
Member

Choose a reason for hiding this comment

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

AppBundle refers to a directory name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! I've changed it to AppBundle/ to make it even more clear

Copy link
Member

Choose a reason for hiding this comment

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

👍

@xabbuh
Copy link
Member

xabbuh commented Jan 17, 2015

👍 (besides the two minor things I commented on)

@wouterj wouterj force-pushed the remove_literal_bundle_names branch from 2f95773 to f361c87 Compare January 23, 2015 18:15
@wouterj
Copy link
Member Author

wouterj commented Jan 23, 2015

@weaverryan before merging, I first want your opinion on this :)

the ``AcmeHelloBundle`` bundle. This helps you specify the path to the resource
without worrying later if you move the ``AcmeHelloBundle`` to a different
directory.
file. The special ``@AcmeHello`` syntax resolves the directory path of the
Copy link
Member

Choose a reason for hiding this comment

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

This is (and was) a typo, right? Should be @AcmeHelloBundle

Copy link
Member

Choose a reason for hiding this comment

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

Fixed at sha: b624100 - someone tackle me if I'm missing something :)

@weaverryan
Copy link
Member

@wouterj Hmm, I like your point - the literals make things stand out, which can be bad for reading. So I definitely support this! In some cases, the bundle is referring to a directory, and in that case it should still have the literals. But I noticed that you did exactly this in your PR, so nice work :).

@weaverryan weaverryan merged commit f361c87 into symfony:2.3 Jan 25, 2015
weaverryan added a commit that referenced this pull request Jan 25, 2015
This PR was merged into the 2.3 branch.

Discussion
----------

Removed literals for bundle names

*Note: Most of it was automated and I didn't review it closely yet (since I didn't know the general opinion on this topic), so there can be some faulty changes.*

I always like to use as less literals as possible, as I think it makes sentences harder to understand. That's why I strongly believe that literals should exclusively be used for code. As a bundle name is not code, I propose to remove all literals around bundle names.

| Q   | A
| --- | ---
| Doc fix? | yes
| New docs? | nope
| Applies to | all
| Fixed tickets | -

Commits
-------

f361c87 Removed literals for bundle names
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.

5 participants