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

Show config actions and a result as Console output #8

Conversation

christopher-hopper
Copy link
Contributor

Proposed Changes

  1. Output to console config changes being made
  2. When -v (verbose) flag is used, also output PHP CodeSniffer config command results

Related Issues

Fixes #7

src/Plugin.php Outdated
$arguments = ['--config-set', 'installed_paths', implode(',', $this->installedPaths)];
$paths = implode(',', $this->installedPaths);
$arguments = ['--config-set', $configKey, $paths];
$this->io->write(sprintf('PHP CodeSniffer Config <info>%s</info> <comment>set to</comment> <info>%s</info>', $configKey, $paths));
Copy link
Member

Choose a reason for hiding this comment

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

As a matter of cleanup, I would suggest assigning the message to a separate variable (instead of calling $this->io->write()) and placing the call to io->write outside of the if/else.

Copy link
Contributor

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Thanks for your addition!
We are willing to merge this one, but have some minor improvements.
Could you please check them?

After updating this PR, we will be happy to merge it 👍

src/Plugin.php Outdated
@@ -141,18 +141,29 @@ private function loadInstalledPaths()
*/
private function saveInstalledPaths()
{
// By default we delete the installed paths
$arguments = ['--config-delete', 'installed_paths'];
$configKey = 'installed_paths';
Copy link
Contributor

Choose a reason for hiding this comment

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

This configKey (installed_paths) is used multiple times in this class / Plugin (e.g. line 121).

I would really like to keep a single style. Right now it used as a variable here, while being used directly elsewhere. I would suggest on moving it into a class constant.

src/Plugin.php Outdated
$arguments = ['--config-set', $configKey, $paths];
$this->io->write(sprintf('PHP CodeSniffer Config <info>%s</info> <comment>set to</comment> <info>%s</info>', $configKey, $paths));
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Violates PRS-2: 5.1. if, elseif, else.
Please correct and place else on the same line as the closing bracket of the above if statement.

@christopher-hopper
Copy link
Contributor Author

christopher-hopper commented Feb 5, 2017

Thanks for the review @frenck @Potherca -- I've pushed updates addressing your suggestions. Love your work.

I was a little unsure about the constant name. Do I go for the better readability (ie. PHPCS_CONFIG_KEY_INSTALLED_PATHS), or more brevity (ie. CONFIG_KEY). Landed somewhere in the middle I hope. Naming things: so difficult sometimes.

@frenck frenck merged commit f866c3e into PHPCSStandards:master Feb 6, 2017
@christopher-hopper christopher-hopper deleted the feature/install-plugin-feedback branch February 6, 2017 22:34
@Potherca
Copy link
Member

Potherca commented Feb 7, 2017

@christopher-hopper To (mis)quote a poem by T.S. Eliot:

The Naming of Cats a constant is a difficult matter,
It isn't just one of your holiday games;
You may think at first I'm as mad as a hatter
When I tell you, a cat a constant must have THREE DIFFERENT 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.

3 participants