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 more specific sub command error handling #6038

Conversation

@TylerSustare
Copy link
Contributor

commented Apr 22, 2019

What did you implement:

Closes #6035

How did you implement it:

Before sending the user to getCommandSuggestion, if the previous commands are valid, return a message telling the user how better to self-correct. Admittedly this message is only to tell the user how to run a command that will give him/her a better error message. Most error handling is handled well in the serverless framework. However, for these niche cases, I feel this is a slight improvement.

How can we verify it:

This is only handled in error cases where the framework is basically saying "We don't know what you want". I couldn't find any tests for this case to modify.
Current handling:
image

With this change:
image

This change does not conflict with any other validation methods. For example validateOptions() still runs as anticipated since this error case is only with commands or aliases.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

Copy link
Member

left a comment

Thanks for working on this @TylerSustare 👍

Can you add a unit test for this so that we can be sure that it won't be altered accidentally in the future? Thanks!

@pmuens pmuens self-assigned this Apr 23, 2019
@pmuens pmuens added this to In progress in Serverless via automation Apr 23, 2019
describe('#getCommand()', () => {
beforeEach(() => {
pluginManager.addPlugin(SynchronousPluginMock);
pluginManager.serverless.cli.loadedCommands = {

This comment has been minimized.

Copy link
@TylerSustare

TylerSustare Apr 23, 2019

Author Contributor

@pmuens Absolutely! Good call on adding a test for it.
When adding the loadedCommands I would normally put this JS object in a fixtures file but it didn't look like this project really uses fixtures. So I put it in line with the test. Let me know if there is a preferred way to do this.

This comment has been minimized.

Copy link
@pmuens

pmuens Apr 24, 2019

Member

Sounds good. Yes, using this inline should be ok 👍

@pmuens pmuens added pr/in-review and removed pr/in-progress labels Apr 24, 2019
@pmuens
pmuens approved these changes Apr 24, 2019
Copy link
Member

left a comment

Just tested it and it works fine! Thanks for working on this @TylerSustare 👍

LGMT :shipit:

describe('#getCommand()', () => {
beforeEach(() => {
pluginManager.addPlugin(SynchronousPluginMock);
pluginManager.serverless.cli.loadedCommands = {

This comment has been minimized.

Copy link
@pmuens

pmuens Apr 24, 2019

Member

Sounds good. Yes, using this inline should be ok 👍

Serverless automation moved this from In progress to Reviewer approved Apr 24, 2019
@pmuens pmuens merged commit 9258801 into serverless:master Apr 24, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Serverless automation moved this from Reviewer approved to Done Apr 24, 2019
@pmuens pmuens changed the title Add more specific sub command error handling. Add more specific sub command error handling Apr 24, 2019
@pmuens pmuens added this to the 1.42.0 milestone Apr 24, 2019
@TylerSustare TylerSustare deleted the TylerSustare:increase-sls-plugin-install-error-handling branch Aug 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Serverless
  
Done
2 participants
You can’t perform that action at this time.