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

Display shell help message with `out` instead of `err` when missing method #11179

Merged
merged 2 commits into from Sep 13, 2017

Conversation

Projects
None yet
6 participants
@littleylv
Contributor

littleylv commented Sep 13, 2017

When i run bin/cake orm_cache (without any methods), it shows error message (red font color):

<info>Usage:</info>
cake orm_cache [subcommand] [-c default] [-h] [-q] [-v] [<name>]

<info>Subcommands:</info>

build  Build all metadata caches for the connection. If a table name is
       provided, only that table will be cached.
clear  Clear all metadata caches for the connection. If a table name is
       provided, only that table will be removed.

To see help on a subcommand use <info>`cake orm_cache [subcommand] --help`</info>

<info>Options:</info>

--connection, -c  The connection to build/clear metadata cache data for.
                  <comment>(default: default)</comment>
--help, -h        Display this help.
--quiet, -q       Enable quiet output.
--verbose, -v     Enable verbose output.

<info>Arguments:</info>

name  A specific table you want to clear/refresh cached data for.
      <comment>(optional)</comment>
@dereuromark

This comment has been minimized.

Show comment
Hide comment
@dereuromark

dereuromark Sep 13, 2017

Member

I discovered the same the last days.
Might also be intentional, as without arguments the command is no longer valid without an explicit default main command method.
I guess that one always has to run bin/cake my_command -h with help flag to get the desired overview and help output.
Without it it seems to be considered incomplete now.

Member

dereuromark commented Sep 13, 2017

I discovered the same the last days.
Might also be intentional, as without arguments the command is no longer valid without an explicit default main command method.
I guess that one always has to run bin/cake my_command -h with help flag to get the desired overview and help output.
Without it it seems to be considered incomplete now.

@dereuromark dereuromark added this to the 3.5.3 milestone Sep 13, 2017

@markstory markstory self-assigned this Sep 13, 2017

@markstory markstory added the console label Sep 13, 2017

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 13, 2017

Codecov Report

Merging #11179 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #11179      +/-   ##
============================================
+ Coverage     93.15%   93.15%   +<.01%     
- Complexity    12978    12979       +1     
============================================
  Files           437      437              
  Lines         33619    33622       +3     
============================================
+ Hits          31317    31320       +3     
  Misses         2302     2302
Impacted Files Coverage Δ Complexity Δ
src/Console/Shell.php 94.97% <100%> (+0.02%) 96 <0> (ø) ⬇️
src/Cache/Engine/FileEngine.php 89.07% <0%> (-1.1%) 73% <0%> (ø)
src/Console/CommandRunner.php 92.45% <0%> (+0.29%) 19% <0%> (+1%) ⬆️
src/Cache/CacheEngine.php 93.61% <0%> (+4.25%) 19% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6829ddd...6da6d0b. Read the comment docs.

codecov-io commented Sep 13, 2017

Codecov Report

Merging #11179 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #11179      +/-   ##
============================================
+ Coverage     93.15%   93.15%   +<.01%     
- Complexity    12978    12979       +1     
============================================
  Files           437      437              
  Lines         33619    33622       +3     
============================================
+ Hits          31317    31320       +3     
  Misses         2302     2302
Impacted Files Coverage Δ Complexity Δ
src/Console/Shell.php 94.97% <100%> (+0.02%) 96 <0> (ø) ⬇️
src/Cache/Engine/FileEngine.php 89.07% <0%> (-1.1%) 73% <0%> (ø)
src/Console/CommandRunner.php 92.45% <0%> (+0.29%) 19% <0%> (+1%) ⬆️
src/Cache/CacheEngine.php 93.61% <0%> (+4.25%) 19% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6829ddd...6da6d0b. Read the comment docs.

@littleylv

This comment has been minimized.

Show comment
Hide comment
@littleylv

littleylv Sep 13, 2017

Contributor

Or should we show some error message before output help:

$this->err('foo');
$this->out($this->OptionParser->help($command));

see: CommandRunner.php#L166-L169

Contributor

littleylv commented Sep 13, 2017

Or should we show some error message before output help:

$this->err('foo');
$this->out($this->OptionParser->help($command));

see: CommandRunner.php#L166-L169

@dereuromark

This comment has been minimized.

Show comment
Hide comment
@dereuromark

dereuromark Sep 13, 2017

Member

Looks like that would make sense. We should at least make sure we keep the exit code of 1 here.

Member

dereuromark commented Sep 13, 2017

Looks like that would make sense. We should at least make sure we keep the exit code of 1 here.

@markstory markstory merged commit a3c1182 into cakephp:master Sep 13, 2017

5 checks passed

codecov/patch 100% of diff hit (target 93.15%)
Details
codecov/project 93.15% (+<.01%) compared to 6829ddd
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
stickler-ci No lint errors found.
@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Sep 13, 2017

Member

Thanks!

Member

markstory commented Sep 13, 2017

Thanks!

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Sep 13, 2017

I don't think this change is correct; when usage (help) output is printed due to missing arguments on Linux, that output should be send to stdErr to prevent the output from being piped to other commands when redirecting output.

I did a quick search to provide more information, and found this; https://www.jstorimer.com/blogs/workingwithcode/7766119-when-to-use-stderr-instead-of-stdout, but there should be other resources explaining this

thaJeztah commented Sep 13, 2017

I don't think this change is correct; when usage (help) output is printed due to missing arguments on Linux, that output should be send to stdErr to prevent the output from being piped to other commands when redirecting output.

I did a quick search to provide more information, and found this; https://www.jstorimer.com/blogs/workingwithcode/7766119-when-to-use-stderr-instead-of-stdout, but there should be other resources explaining this

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Sep 14, 2017

Member

@thaJeztah This change was to restore behaviour that has existed for quite some time. The reasons you cited were part of the reason I has used stderr. But those changes have caused people grief.

Member

markstory commented Sep 14, 2017

@thaJeztah This change was to restore behaviour that has existed for quite some time. The reasons you cited were part of the reason I has used stderr. But those changes have caused people grief.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Sep 14, 2017

@markstory thanks for clarifying - my eye fell on this PR's title, and I know there's sometimes confusion around this, so thought to add the information above, but understand the reason to change if it broke people 👍

thaJeztah commented Sep 14, 2017

@markstory thanks for clarifying - my eye fell on this PR's title, and I know there's sometimes confusion around this, so thought to add the information above, but understand the reason to change if it broke people 👍

@littleylv littleylv deleted the littleylv:missing-method branch Sep 14, 2017

@rchavik

This comment has been minimized.

Show comment
Hide comment
@rchavik

rchavik Sep 14, 2017

Member

I too disagree with this change.

I think a better solution is:

diff --git a/src/Console/Shell.php b/src/Console/Shell.php
index d513038..8bd52aa 100644
--- a/src/Console/Shell.php
+++ b/src/Console/Shell.php
@@ -508,7 +508,7 @@ class Shell
             return $this->main(...$this->args);
         }
 
-        $this->err($this->OptionParser->help($command));
+        $this->_io->err($this->OptionParser->help($command));
 
         return false;
     }

This also correctly handles the <info /> tags as oppose to using the err() or out() wrapper

Member

rchavik commented Sep 14, 2017

I too disagree with this change.

I think a better solution is:

diff --git a/src/Console/Shell.php b/src/Console/Shell.php
index d513038..8bd52aa 100644
--- a/src/Console/Shell.php
+++ b/src/Console/Shell.php
@@ -508,7 +508,7 @@ class Shell
             return $this->main(...$this->args);
         }
 
-        $this->err($this->OptionParser->help($command));
+        $this->_io->err($this->OptionParser->help($command));
 
         return false;
     }

This also correctly handles the <info /> tags as oppose to using the err() or out() wrapper

@dereuromark

This comment has been minimized.

Show comment
Hide comment
@dereuromark

dereuromark Sep 14, 2017

Member

@rchavik You can make a follow up PR :)

Member

dereuromark commented Sep 14, 2017

@rchavik You can make a follow up PR :)

@littleylv

This comment has been minimized.

Show comment
Hide comment
@littleylv

littleylv Sep 14, 2017

Contributor

@rchavik 👍

Contributor

littleylv commented Sep 14, 2017

@rchavik 👍

rchavik added a commit that referenced this pull request Sep 14, 2017

Revert "Merge pull request #11179 from littleylv/missing-method"
This reverts commit a3c1182, reversing
changes made to 7c8ddbe.

rchavik added a commit that referenced this pull request Sep 14, 2017

Revert "Merge pull request #11179 from littleylv/missing-method"
This reverts commit a3c1182, reversing
changes made to 7c8ddbe.

rchavik added a commit that referenced this pull request Sep 14, 2017

rchavik added a commit that referenced this pull request Sep 15, 2017

Partial revert "Merge pull request #11179 from littleylv/missing-method"
This reverts commit a3c1182, reversing
changes made to 7c8ddbe with exception
of 1 line

rchavik added a commit that referenced this pull request Sep 15, 2017

o0h added a commit to o0h/cakephp that referenced this pull request Nov 16, 2017

Partial revert "Merge pull request cakephp#11179 from littleylv/missi…
…ng-method"

This reverts commit a3c1182, reversing
changes made to 7c8ddbe with exception
of 1 line

o0h added a commit to o0h/cakephp that referenced this pull request Nov 16, 2017

o0h added a commit to o0h/cakephp that referenced this pull request Dec 30, 2017

Partial revert "Merge pull request cakephp#11179 from littleylv/missi…
…ng-method"

This reverts commit a3c1182, reversing
changes made to 7c8ddbe with exception
of 1 line

o0h added a commit to o0h/cakephp that referenced this pull request Dec 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment