Added standard bundle installation instructions #4163

javiereguiluz · 2014-08-22T11:12:57Z

Q	A
Doc fix?	no
New docs?	yes
Applies to	-
Fixed tickets	#4157

Note: if you think that this draft is awful, please say it so and we'll rewrite it from scratch.

xabbuh · 2014-08-22T15:44:17Z

Basically, I like the idea and how you wrote this section. I'm not sure if it is good to place it at the bottom of the chapter though. Doesn't it belong to the "Documentation" section? I also think that it's worth to merge this into 2.3.

xabbuh · 2014-08-22T15:46:12Z

cookbook/bundles/best_practices.rst

+In order to ease the installation process of third-party bundles, we propose
+you to use the following standardized instructions in your ``README`` file:
+
+.. code-block::


We need a language here. I guess markdown doesn't work. Can you check whether or not this renders properly when you use rst?

javiereguiluz · 2014-08-22T15:53:24Z

@xabbuh thanks for your review! I've switched the "installation template" to a sidebar and I've used the .. raw RST directive to output the contents verbatim.

javiereguiluz · 2014-08-22T15:55:23Z

My main concern is this command:

$ composer require "<vendor>/<bundle-name>:dev-master"

Which version should we tell bundle creators to include in that command? Lots of them are including @stable right now.

stof · 2014-08-22T16:11:01Z

@javiereguiluz I think the bundle doc should use composer require <package name> '~1' if they are in their 1.x version (you could go to a more precise requirement by giving a minimal minor version in the constraint, but this would be painful to maintain).
This would mean that bundles would have to update the installation doc only when they move to a new major version. But in such case, they already have to update their doc.

dev-master is even worse than @stable:

it is also unbound
it forbids using releases, forcing to always rely on potentially-unstable code

stof · 2014-08-22T16:11:46Z

cookbook/bundles/best_practices.rst

+In order to ease the installation process of third-party bundles, we propose
+you to use the following standardized instructions in your ``README`` file.
+
+.. sidebar:: Installation Instructions Template


you should either add a note saying this is a rST template, or give also a Markdown template. Most projects are using Markdown for the readme on github.

You are right. RST is the official format for the bundle documentation, but 99% of bundles hosted on GitHub use README.md instead of README.rst. I've switched the template to Markdown.

well, the best practice in the doc was written at a time where symfony.com was expected to have a repository of bundles with their doc being rendered (KnpBundles appeared as a third-party solution to this problem because the official tool announced by Fabien was not yet there, and never became a reality given we then choose composer meaning that KnpBundles and Packagist can probably be enough and people can use libraries which are not bundles). The de-facto standard then became Markdown because everyone is using Github (well, mostly at least) and it used to suck at rendering rST (3 years ago when building the Symfony2 ecosystem, the code-block directive was not supported by their rST renderer for instance) while Markdown was well rendered

To make myself more clear, this is what I would like to have:

but ..raw does not achieve this. It let you pass unprocessed context (i.e. either html or latex or other format, which is why it asks you for a format btw: you have to provide it for each output format).
What you expect in your image is still a <pre> tag in HTML, and it is technically a code-block (so <pre><code> is more semantic).
the only difference with the rendering of ..code-block is that you don't want line numbers (this is fine, Sphinx has an option to omit them in its directive) and a different background color (and this is a matter of theming) But I could totally live with the code being on a block background, and people will be able to copy-paste it.
See http://symfony.com/doc/current/contributing/code/patches.html#make-a-pull-request and you will see that the code snippet containing the pull-request template can totally be copy-pasted (we did not even bothered about removing line numbers on it)

You are right! We just need a simple .. code-block:: text directive to output the Markdown content "as is".

raulfraile · 2014-08-22T18:12:43Z

cookbook/bundles/best_practices.rst

+
+        This command requires to have installed Composer globally, as explained
+        in the [installation chapter](https://getcomposer.org/doc/00-intro.md)
+        of Composer documentation.


wouldn't be "have Composer installed globally"?

And we could also use "to have Composer globally installed". I'm not sure what to do. We need the help of some native speaker.

I agree with @raulfraile. I would also remove everything after the comma and directly link "Composer globally installed".

javiereguiluz · 2014-08-22T18:40:00Z

I think it's time to make a decision: @weaverryan @wouterj @xabbuh @stof do you think that this documentation is good enough to start sending PR to all bundles' repositories during the second DX Hackday? Or do you prefer to take more time to think more about this?

xabbuh · 2014-08-22T19:50:01Z

cookbook/bundles/best_practices.rst

@@ -194,6 +194,59 @@ Extensive documentation should also be provided in the
 the ``Resources/doc/`` directory; the ``Resources/doc/index.rst`` file is
 the only mandatory file and must be the entry point for the documentation.

+Installation instructions


"Instructions" should be upper-cased.

xabbuh · 2014-08-22T19:58:42Z

cookbook/bundles/best_practices.rst

+        }
+        ```
+
+    Optionally you can add *Step 3*, *Step 4*, etc. to explain other required


Add a comma after "optionally".

xabbuh · 2014-08-22T21:26:20Z

cookbook/bundles/best_practices.rst

+                // ...
+                new <vendor>\<bundle-name>\<bundle-long-name>(),
+            );
+        }


The return statement is missing.

I usually add a // ... before this line too

If we add the return statements, I don't think that this is necessary. Usually you would simply add the new bundle at the end of the array and then return the $bundles array, won't you? So, having another placeholder there might lead to some confusion.

javiereguiluz · 2014-08-23T10:31:10Z

What do you think about the last change?

Original text

Use Composer to add the bundle as a new dependency of your project:

    $ composer require <package-name> "~1"

Proposal

Open a command console, enter your project directory and execute the
following command to download the latest stable version of this bundle:

    $ composer require <package-name> "~1"

stof · 2014-08-23T10:38:49Z

@javiereguiluz You should add a note after the code block to tell authors to update the constraint according to the major version of the bundle (so that they know they should change it).
And for it to work fine, the master branch should have a branch-alias (so that the master branch can match such requirement without using dev-master as constraint). So I would suggest adding a best practice for bundle telling they should use a branch alias (and linking to https://getcomposer.org/doc/articles/aliases.md)

weaverryan · 2014-08-25T18:19:43Z

cookbook/bundles/best_practices.rst

+    $ composer require <package-name> "~1"
+    ```
+
+    This command requires to have Composer installed globally, as explained


... requires you to have Composer

weaverryan · 2014-08-25T18:23:14Z

@javiereguiluz Sorry for the delay - my eyes weren't on the docs during the hack day. I added some comments, but then I think this is ready! I also agree with @stof's comment, but I think it should be a different section or can be a different PR altogether.

There are 2 more changes that are coming (composer/composer#3096 and the bundle:install-like command), but I think we should use this and start making PR's. Then we'll just do another update later when we have these changes :).

Thanks!

javiereguiluz · 2014-09-01T13:47:59Z

Should I make any other change to this PR before merging it?

javiereguiluz · 2014-09-08T11:26:48Z

This issue is labeled as In progress, but I don't know what is left to be done. Any clue? Thanks!

xabbuh · 2014-09-08T12:02:49Z

Looks fine to me. 👍

weaverryan · 2014-09-16T01:58:10Z

@javiereguiluz I think this is Finished. Now we will also have @xabbuh's help in properly labeling things. Once it's finished, I'm looking for those so I can merge them quickly :).

BUT, your branch is missing! Do you still have it locally? If not, I'll merge via the interface. But the internal merge tool requires your branch to be there. Let me know.

Thanks!

stof · 2014-09-16T03:40:37Z

@weaverryan the GH tool provided by Fabien does not need the branch to be there. It gets the code from the special ref available in the symfony repo for PRs, not from the branch ref in the fork.
Are you using a different tool for the doc ?

javiereguiluz · 2014-09-16T10:52:11Z

@weaverryan I no longer have the branch locally :(

weaverryan · 2014-09-16T12:44:51Z

@stof, I'm using gh, and I just made sure I was on the latest version. It says Fetching javiereguiluz repository before failing when running a log on master..javiereguiluz/bundle_installation_instructions.

Is it possible that it doesn't grab from the docs ref when using --switch? I don't want to talk about gh here, I just want to know if I'll need to merge via the GitHub interface for this PR and if we should follow up with gh if this is a bug.

Thanks!

wouterj · 2014-09-16T12:45:43Z

@weaverryan I'll make a PR shortly with the correct branch

wouterj · 2014-09-16T12:46:30Z

see #4236

javiereguiluz · 2014-09-16T13:01:37Z

@weaverryan @stof there is nothing wrong with the tool you use. The branch is not available because I'm stupid and I deleted my symfony-docs fork :(

weaverryan · 2014-09-16T13:03:56Z

@javiereguiluz no worries :). Stof was saying that the tool can actually fetch the missing code from a different location (similar to what the GH merge button must do behind the scenes). But we have a new PR now, so I'll merge from there.

Thanks!

This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes #4236). Discussion ---------- Javiereguiluz bundle install instructions Replaces #4163 Commits ------- 7097070 Added the fixes suggested by Ryan 35dddba Reworded a bit the installation instructions 7a6644b Removed the sidebar and made some minor tweaks 41ce2a5 Minor fixes and tweaks 4b29de2 More tweaks and fixes 322ec60 Removed the first person perspective and other minor fixes c39f7d3 Fixed some code formatting ed96447 More improvements based on reviewers' comments 21ce2c0 Fixed minor typo 11db860 Applied all the fixes and suggestions made by reviewers b297d29 First draft of the bundle installation instructions

wouterj · 2014-09-16T13:07:06Z

@javiereguiluz github always keeps branches of PRs in the main repository, in order to always show the diff of the PR (even when the branch was removed). You can fetch that by using git fetch origin pull/NR/head:BRANCHNAME (where NR is the number of the PR and BRANCHNAME is the name of the branch you want to use locally).

stof · 2014-09-16T14:55:58Z

I'm using gh, and I just made sure I was on the latest version. It says Fetching javiereguiluz repository before failing when running a log on master..javiereguiluz/bundle_installation_instructions

This looks weird. GH does not run such commands. Are you sure you run the code from the sensiolabs/gh repo ?

wouterj · 2014-09-16T15:17:22Z

I think fabien sent the tool to ryan back in January 2014, so I don't think it's very up to date (not sure though)

stof · 2014-09-16T15:23:40Z

well, I know for sure that Ryan has access to the git repo to get the latest version of the tool

weaverryan · 2014-10-01T13:41:26Z

@stof you're right - I updated locally, but was still using my old binary. So, my fault!

First draft of the bundle installation instructions

b297d29

xabbuh reviewed Aug 22, 2014
View reviewed changes

Applied all the fixes and suggestions made by reviewers

11db860

Fixed minor typo

21ce2c0

stof reviewed Aug 22, 2014
View reviewed changes

More improvements based on reviewers' comments

ed96447

raulfraile reviewed Aug 22, 2014
View reviewed changes

Fixed some code formatting

c39f7d3

xabbuh reviewed Aug 22, 2014
View reviewed changes

Removed the first person perspective and other minor fixes

322ec60

xabbuh reviewed Aug 22, 2014
View reviewed changes

More tweaks and fixes

4b29de2

xabbuh reviewed Aug 22, 2014
View reviewed changes

Reworded a bit the installation instructions

35dddba

weaverryan reviewed Aug 25, 2014
View reviewed changes

xabbuh mentioned this pull request Aug 25, 2014

Update installation instructions maschmann/TranslationLoaderBundle#21

Closed

Added the fixes suggested by Ryan

7097070

xabbuh added Status: Reviewed and removed Status: Needs Work labels Sep 16, 2014

wouterj mentioned this pull request Sep 16, 2014

Javiereguiluz bundle install instructions #4236

Closed

wouterj closed this Sep 16, 2014

Uh oh!

Added standard bundle installation instructions #4163

Added standard bundle installation instructions #4163

Uh oh!

Conversation

javiereguiluz commented Aug 22, 2014

Uh oh!

xabbuh commented Aug 22, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javiereguiluz commented Aug 22, 2014

Uh oh!

javiereguiluz commented Aug 22, 2014

Uh oh!

stof commented Aug 22, 2014

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!

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!

javiereguiluz commented Aug 22, 2014

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!

javiereguiluz commented Aug 23, 2014

Uh oh!

stof commented Aug 23, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weaverryan commented Aug 25, 2014

Uh oh!

javiereguiluz commented Sep 1, 2014

Uh oh!

javiereguiluz commented Sep 8, 2014

Uh oh!

xabbuh commented Sep 8, 2014

Uh oh!

weaverryan commented Sep 16, 2014

Uh oh!

stof commented Sep 16, 2014

Uh oh!

javiereguiluz commented Sep 16, 2014

Uh oh!

weaverryan commented Sep 16, 2014

Uh oh!

wouterj commented Sep 16, 2014

Uh oh!

wouterj commented Sep 16, 2014

Uh oh!

javiereguiluz commented Sep 16, 2014

Uh oh!

weaverryan commented Sep 16, 2014

Uh oh!

wouterj commented Sep 16, 2014

Uh oh!

stof commented Sep 16, 2014