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

Add install/update instructions for PHIVE to the README #3848

Merged
merged 1 commit into from Jul 5, 2018
Merged

Add install/update instructions for PHIVE to the README #3848

merged 1 commit into from Jul 5, 2018

Conversation

SpacePossum
Copy link
Contributor

closes #3838

README.rst Outdated

.. code-block:: bash

$ phive install php-cs-fixer # use `--global` for global install, not recommended
Copy link
Member

Choose a reason for hiding this comment

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

I would drop , not recommended. It's opinionated, and even while I agree with it, we don't put that for other ways of installation, don't we ?
Here, i brings the confusion that installing sth with PHIVE globally is not recommended, it's not true.
We simply don't recommend to install dev-tools globally (as their versions depends on project), regardless which way of installation was chosen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so maybe just drop the comment completely?

Copy link
Member

Choose a reason for hiding this comment

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

i would keep it

@keradus keradus added this to the 2.12.2 milestone Jul 2, 2018
@PHP-CS-Fixer PHP-CS-Fixer deleted a comment from keradus Jul 4, 2018
@SpacePossum SpacePossum added the RTM Ready To Merge label Jul 4, 2018
@@ -131,6 +131,15 @@ protected function execute(InputInterface $input, OutputInterface $output)

$ brew install php-cs-fixer

Locally (PHIVE)
~~~~~~~~~~~~~~~~~~~
Copy link
Contributor

Choose a reason for hiding this comment

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

Why so long? Other underline-ish lines are with the length of the text above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll admit, the lengthy ~ is a result of copying another (sub)header, I'll update soon, thanks for the sharp review 👍 :)

@SpacePossum SpacePossum changed the title Add install/update using PHIVE instructions to README Add install/update instructions for PHIVE to the README Jul 5, 2018
@SpacePossum SpacePossum merged commit e91b75b into PHP-CS-Fixer:2.12 Jul 5, 2018
SpacePossum added a commit that referenced this pull request Jul 5, 2018
…SpacePossum)

This PR was merged into the 2.12 branch.

Discussion
----------

Add install/update instructions for PHIVE to the README

closes #3838

Commits
-------

e91b75b Add install/update using PHIVE instructions to README
@SpacePossum SpacePossum removed the RTM Ready To Merge label Jul 5, 2018
@SpacePossum SpacePossum deleted the 2_12_install_by_PHIVE_instructions_in_README branch July 5, 2018 07:22
@@ -124,6 +133,13 @@ You can update ``php-cs-fixer`` through this command:

$ brew upgrade php-cs-fixer

Locally (PHIVE)
~~~~~~~~~~~~~~~~~~~
Copy link
Contributor

@kubawerlos kubawerlos Jul 5, 2018

Choose a reason for hiding this comment

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

No so fast, this one is also too long ;)

Oh, already merged :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh no, I really really really thought I had this covered, f**k me, this is a lesson learned on my "I wished wasn't needed "-list :(
aaarrggh I'm sorry man, you called me out on it before and still I messed it up, f**k me :(

Copy link
Member

Choose a reason for hiding this comment

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

no worry. instead of fixing that, let us run rst / md linters on CI :D

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

3 participants