Skip to content
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

Fix redundant command line sample #6190

Merged
merged 1 commit into from
Feb 2, 2016
Merged

Fix redundant command line sample #6190

merged 1 commit into from
Feb 2, 2016

Conversation

slelievre
Copy link

Q A
Doc fix? yes
New docs? no
Applies to all
Fixed tickets #6189

Removed option --with-dependencies from the first command line example for composer upgrade.

@wouterj
Copy link
Member

wouterj commented Jan 27, 2016

Hi @sylozof!

Can you please explain why you made this change? Without this option, Composer will only update the specified package. This means that dependencies aren't updated. Updating to Symfony 2.8 resulted in errors, as it needed a twig update as well. This is why we introduced this option.

Anyway, as you took the time to submit an update, there must be some reason (maybe this is the best fix, or we should add some note/comment).

@slelievre
Copy link
Author

Hi @wouterj !

Maybe I was two concise in the PR but I have given more explanations in the related issue #6189. The fact is that when you look at the concerned page for major version upgrade, section 2), you have twice the same command line sample, each with the --with-dependencies option (see the screen cap below).
sf-doc-screenshot

So I thought it was redundant and my PR aimed at having something similar to what is found in the page for minor version upgrade, same section 2) : one sample command line without the option, and the next one with the option.

But maybe my proposal was not relevant if an upgrade to a major version always need the --with-dependencies option. In that case, is the following paragraph talking about dependency errors still need to be here ?

I hope I was clear, don't hesitate to ask for clarifications again.

Also, I'm kind of a beginner with Git and Github, so I'm not sure I'm doing my suggestion the right way, even if I have read the guidelines for contribution before.

@xabbuh
Copy link
Member

xabbuh commented Jan 27, 2016

From what I understand from your issue description, the change looks valid to me. However, it seems that the pull request contains a lot of changes that a unrelated to the changes you did. Would you be able to rebase your changes on the 2.3 branch so that we are able to merge them?

@slelievre
Copy link
Author

Well, I'm not very familiar with Git and Github but I will try anyway.

@javiereguiluz
Copy link
Member

@sylozof this guide explains how to contribute to Symfony Docs step by step and from scratch: http://symfony.com/doc/current/contributing/documentation/overview.html

Could you please follow that guide to tell us if it's OK or if it's missing some information? And if you have any question or problem, please ask us for help. Thanks!

@slelievre slelievre changed the title Patch 1 Fix redundant command line sample Jan 28, 2016
@wouterj
Copy link
Member

wouterj commented Jan 28, 2016

@sylozof if you can't do it, just tell us and we'll do it for your (while making sure you still get credits for your work). The GIT commands you have to run is something like:

# import the project in your local filesystem
$ git clone https://github.com/symfony/symfony-docs
$ cd symfony-docs

# add your fork as a remote
$ git remote add sylozof https://sylozof@github.com/sylozof/symfony-docs

# fetch the branches from your remote
$ git fetch sylozof

# checkout the branch related to this PR
$ git checkout -b patch-1 sylozof/patch-1

# make the changes (in this case, rebasing from master branch to 2.3)
$ git rebase --onto origin/2.3 origin/master patch-1

# pushing to github (the -f means force and is required as we rebased, don't include this when doing normal pushes in the future)
$ git push -f sylozof

@slelievre
Copy link
Author

@javiereguiluz @wouterj thanks a lot for your help. I will try to do it by myself, because learning a new thing is always a good thing.

I'm a bit in a hurry right now but I will do it within the next days. Thanks again.

Removed option '--with-dependencies' from the first command line example for upgrade.
@slelievre
Copy link
Author

Ok, I've followed the different steps. I'm not sure I've grasped every subtleties of what I've done, but I got no errors so I think everything got well.

Don't hesitate to notify me if something else needs to be done.

And thanks again for your help.

@xabbuh xabbuh merged commit c8618e1 into symfony:2.3 Feb 2, 2016
xabbuh added a commit that referenced this pull request Feb 2, 2016
This PR was merged into the 2.3 branch.

Discussion
----------

Fix redundant command line sample

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | all
| Fixed tickets | #6189

Removed option ``--with-dependencies`` from the first command line example for composer upgrade.

Commits
-------

c8618e1 Update major_version.rst
@xabbuh
Copy link
Member

xabbuh commented Feb 2, 2016

Great job! Thank you @sylozof!

@slelievre
Copy link
Author

You're welcome @xabbuh, but this is more a drop in an ocean than a world-changing contribution :-)

@slelievre slelievre deleted the patch-1 branch August 9, 2016 09:38
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.

None yet

4 participants