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

shell: fix handling of default values #19

Merged
merged 1 commit into from Oct 31, 2015

Conversation

doudou
Copy link
Contributor

@doudou doudou commented Aug 31, 2015

This fixes handling of 'empty input' by having Response detect an
empty line input and feed it to Question#evaluate_response. Another
code path I saw would have required to change all Necromancer
converters to handle this special case ... did not seem appealing.

However, the current Response class assumes that it is possible for
read_input to distinguish between "no value provided" and "empty
line as input". Since I did not see how it was possible, I removed
that capability which ended up changing functionality:

  • read_multiple cannot input empty lines (since an empty line is
    now interpreted as no input)
  • read_email accepts empty email (but returns nil), which incidentally
    fixes default value handling for emails. If an email must be provided
    by the user, then call argument(:required).

This fixes handling of 'empty input' by having Response detect an
empty line input and feed it to Question#evaluate_response. Another
code path I saw would have required to change all Necromancer
converters to handle this special case ... did not seem appealing.

However, the current Response class assumes that it is possible for
read_input to distinguish between "no value provided" and "empty
line as input". Since I did not see how it was possible, I removed
that capability which ended up changing functionality:
 - read_multiple cannot input empty lines (since an empty line is
   now interpreted as no input)
 - read_email accepts empty email (but returns nil), which incidentally
   fixes default value handling for emails. If an email must be provided
   by the user, then call argument(:required).
@doudou
Copy link
Contributor Author

doudou commented Aug 31, 2015

I am more confident about this one. I know the tests do not pass (and that new tests need to be added), but since there are some caveats about the solution, I wanted to have your opinion before going forward with this.

@piotrmurach
Copy link
Owner

Agree with your reasoning behind the 'empty input'. The solution looks pragmatic and i'm happy with it. Would you mind finishing it with tests passing?

@doudou
Copy link
Contributor Author

doudou commented Sep 9, 2015

Would you mind finishing it with tests passing?

Sure. Will do.

piotrmurach added a commit that referenced this pull request Oct 31, 2015
shell: fix handling of default values
@piotrmurach piotrmurach merged commit c399a1b into piotrmurach:master Oct 31, 2015
@doudou
Copy link
Contributor Author

doudou commented Nov 3, 2015

@peter-murach: sorry, I forgot about the tests, very busy months for me.

@doudou
Copy link
Contributor Author

doudou commented Nov 3, 2015

You could have reminded me of them, you know !

@piotrmurach
Copy link
Owner

No worries! You've done the hard work of fixing the issues and I needed to start extracting this out to another component called tty-prompt. If you wish to continue to contribute/use the library the code is over here I've already rewritten few bits and going forward I'm going to give it proper love it deserves.

@doudou doudou deleted the fix_default_response branch November 3, 2015 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants