Minor changes throughout Best Practices for Reusable Bundles #7118

darrylhein · 2016-11-05T22:34:40Z

Cleanup and fixes for parts that were difficult to understand

xabbuh · 2016-11-06T09:43:04Z

bundles/best_practices.rst

@@ -75,7 +76,7 @@ configuration options (see below for some usage examples).
 Directory Structure
 -------------------

-The basic directory structure of an AcmeBlogBundle must read as follows:
+The basic directory structure of an ``AcmeBlogBundle`` must read as follows:


I am against this change. We usually do not enclose bundle names with backticks.

I agree with @xabbuh here. It's a name and not a code-segment. Plus, any changes in text rendering is a minor break of the read flow, so having less in-line code blocks will improve docs readability.

Should we then remove the backticks from the previous section? There are 3 locations where the bundle name is in backticks. ~25% of the references to AcmeBlogBundle on this page are not in backticks while the rest are. (Again, just going for consistency, but don't really care.)

xabbuh · 2016-11-06T09:43:35Z

bundles/best_practices.rst


 The bundle directory is read-only. If you need to write temporary files, store
 them under the ``cache/`` or ``log/`` directory of the host application. Tools
 can generate files in the bundle directory structure, but only if the generated
 files are going to be part of the repository.

-The following classes and files have specific emplacements (some are mandatory
+The following classes and files have specific emplacements (some are mandatory 


This change should be reverted.

xabbuh · 2016-11-06T09:45:27Z

bundles/best_practices.rst


 ``autoload``
-    This information is used by Symfony to load the classes of the bundle. The
+    This information is used by Symfony to load the classes in the bundle. The


This change looks wrong to me. Autoloading is about how classes provided by the bundle are loaded. So we are actually talking about "classes of the bundle".

wouterj · 2016-11-06T11:23:49Z

bundles/best_practices.rst

-`PSR-4`_ interoperability standards for PHP namespaces and class names: it starts
-with a vendor segment, followed by zero or more category segments, and it ends
-with the namespace short name, which must end with a ``Bundle`` suffix.
+A bundle name is also a PHP namespace. The namespace must follow the `PSR-0`_


I'm against this change, the bundle is a PHP namespace. The name of that namespace just follows the bundle name.

I think my reasoning was twofold: (1) the section is called "Bundle Name"; (2) the bundle contains/has a namespace whereas the namespace isn't the bundle. I was confused when reading it (being new to bundles and semi-new to namespaces) as to how the bundle became a namespace. Would it make more sense to do the following: The bundle name is used as the PHP namespace.

OskarStark · 2016-11-15T10:35:24Z

bundles/best_practices.rst

@@ -185,7 +186,7 @@ All classes and functions must come with full PHPDoc.

 Extensive documentation should also be provided in the
 :doc:`reStructuredText </contributing/documentation/format>` format, under
-the ``Resources/doc/`` directory; the ``Resources/doc/index.rst`` file is
+the ``Resources/doc/`` directory. The ``Resources/doc/index.rst`` file is


does it really needs to be RST?

not really. It can in in any readable format. Using rST and putting it in Resources/doc/ is necessary only when rendering the doc on symfony.com

Most bundles are using markdown, as their rendered doc is actually the github UI

@OskarStark @stof I've reworded this section. Does this help?

OskarStark · 2016-11-15T10:39:59Z

bundles/best_practices.rst

@@ -404,7 +404,7 @@ The ``composer.json`` file should include at least the following metadata:

 ``license``
    ``MIT`` is the preferred license for Symfony bundles, but you can use any
-    other license.
+    license.


Maybe we could add a link here. I really like https://tldrlegal.com

https://tldrlegal.com/license/mit-license

Or http://choosealicense.com/ (which is run by GitHub)

Trying to implement comments made on pull request.

javiereguiluz

👍

I made all the changes suggested by reviewers to make this mergeable.

stof · 2016-11-16T18:05:43Z

bundles/best_practices.rst

-the ``Resources/doc/`` directory; the ``Resources/doc/index.rst`` file is
-the only mandatory file and must be the entry point for the documentation.
+Extensive documentation should also be provided in the ``Resources/doc/`` 
+directory (though only required when the documentation is shown on symfony.com).


the sentence in braces looks wrong to me. Providing doc is required when sharing your bundle. the sentence looks like doc is required only when showing it on symfony.com

Putting it inside Resources/doc/ and using rST is what is required when rendering it on symfony.com.
when rendering the doc elsewhere, we could live with the doc being elsewhere (in a docs folder at the root of the repo for instance, which is a convention supported by github pages now) and in any format (as long as you provide a version readable by humans somewhere, and link to it)

stof · 2016-11-16T18:07:15Z

bundles/best_practices.rst

+``Resources/doc/index.md``) is the only mandatory file and must be the entry 
+point for the documentation. The 
+:doc:`reStructuredText (rST) </contributing/documentation/format>` is the most 
+common format as it's what's used to render the documentation on symfony.com


most common is wrong here. Most bundles don't have their doc on symfony.com.

If we were running stats over all bundles, I'm quite sure markdown would be the most common one...

xabbuh · 2017-09-15T10:20:42Z

Thank you @darrylhein.

…les (darrylhein, javiereguiluz, HeahDude) This PR was submitted for the 3.1 branch but it was merged into the 2.7 branch instead (closes #7118). Discussion ---------- Minor changes throughout Best Practices for Reusable Bundles Cleanup and fixes for parts that were difficult to understand Commits ------- c3bd3e6 Update best_practices.rst 5b8698c Better explain the purpose of the "license" Composer metadata 2da0fd8 Reverted the changes about bundle namespaces 7055d6b Removed backticks from bundle names 57fcec0 add mention of symfony.com re folder structure 0ed9c2d reword the documentation section re Resources/doc 4a0632d Reverting previous grammar change 527e16a Removing trailing space fe7795f Minor changes throughout BP for Reusable Bundles

Minor changes throughout BP for Reusable Bundles

d145034

Cleanup and fixes for parts that were difficult to understand

carsonbot added the Status: Needs Review label Nov 5, 2016

darrylhein changed the title ~~Minor changes throughout BP for Reusable Bundles~~ Minor changes throughout Best Practices for Reusable Bundles Nov 6, 2016

xabbuh reviewed Nov 6, 2016

View reviewed changes

wouterj reviewed Nov 6, 2016

View reviewed changes

darrylhein added 2 commits November 8, 2016 16:35

Removing trailing space

f749f0d

Reverting previous grammar change

16d5d39

OskarStark reviewed Nov 15, 2016

View reviewed changes

darrylhein and others added 5 commits November 16, 2016 10:14

reword the documentation section re Resources/doc

592b9d2

Trying to implement comments made on pull request.

add mention of symfony.com re folder structure

99dc9a9

Removed backticks from bundle names

ae29ce8

Reverted the changes about bundle namespaces

07dd260

Better explain the purpose of the "license" Composer metadata

73d2d7c

javiereguiluz approved these changes Feb 14, 2017

View reviewed changes

stof reviewed Feb 14, 2017

View reviewed changes

Update best_practices.rst

5cbb6e8

HeahDude approved these changes Jul 29, 2017

View reviewed changes

HeahDude added Status: Reviewed and removed Status: Needs Review labels Jul 29, 2017

xabbuh closed this Sep 15, 2017

Uh oh!

Minor changes throughout Best Practices for Reusable Bundles #7118

Minor changes throughout Best Practices for Reusable Bundles #7118

Uh oh!

Conversation

darrylhein commented Nov 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wouterj Nov 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OskarStark Nov 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javiereguiluz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Sep 15, 2017

Uh oh!

Uh oh!

wouterj Nov 6, 2016 •

edited

Loading

OskarStark Nov 15, 2016 •

edited

Loading