Skip to content

Show config actions and a result as Console output#8

Merged
frenck merged 4 commits intoPHPCSStandards:masterfrom
christopher-hopper:feature/install-plugin-feedback
Feb 6, 2017
Merged

Show config actions and a result as Console output#8
frenck merged 4 commits intoPHPCSStandards:masterfrom
christopher-hopper:feature/install-plugin-feedback

Conversation

@christopher-hopper
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
{
// By default we delete the installed paths
$arguments = ['--config-delete', 'installed_paths'];
$configKey = 'installed_paths';
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Development

Successfully merging this pull request may close these issues.

3 participants