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

[WIP] Add selected to choice #142

Closed

Conversation

DanielVartanov
Copy link
Contributor

Describe the change

Add selected property to Choice

Why are we doing this?

In PR #141 Piotr suggested

Alternatively, we could extend the Choice with a new state like selected. This would have an added benefit that you could define your choice as selected before the menu is rendered. Then we could remove the @selected collection and replace the call with an already sorted list:
choices.select(&:selected?)

Benefits

It also allows to pre-select default options by passing selected: true option to menu.choice

Drawbacks

  • Breaks a promise of immutability of instances of Choice as described in public API
  • A mixup/confusion between different ways of setting defaults
  • It's still WIP, many edgecases (especially related to disabled options) must be re-analysed.

Requirements

Put an X between brackets on each line if you have done the item:
[X] Tests written & passing locally?
[X] Code style checked?
[X] Rebased with master branch?
[X] Documentation updated?

@DanielVartanov
Copy link
Contributor Author

@piotrmurach just a note: It seems that an immutable entity which describes the choice (name, value, disabled etc) and a mutable entity which describes a presence of a choice in a collection (selected, maybe index etc) are separate entities, though I didn't find a good name for the latter yet they probably need to be separated.

@coveralls
Copy link

coveralls commented Jun 21, 2020

Coverage Status

Coverage increased (+0.01%) to 97.355% when pulling 10d88f7 on DanielVartanov:add-selected-to-choice into b330a32 on piotrmurach:master.

@DanielVartanov
Copy link
Contributor Author

Apologies for the failed test, something must've been wrong while rebasing, I'll double chek

@DanielVartanov
Copy link
Contributor Author

Yes, default help has been changed recent, therefore the test has failed after rebasing. Fixed it now.

Copy link
Owner

@piotrmurach piotrmurach left a comment

Choose a reason for hiding this comment

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

Again I'm reluctant to lose the 'immutability' guarantee for the Choice class. There are few ideas anyhow that I'd like to port over to the gem whether or not we go with this solution.

@@ -2,7 +2,7 @@

module TTY
class Prompt
# An immutable representation of a single choice option from select menu
# A representation of a single choice option from select menu
Copy link
Owner

Choose a reason for hiding this comment

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

Same observation here, I do like the 'immutability' guarantee here. This is especially important when you may want to call the prompt recursively and 'loop' the choice selection.

Comment on lines +51 to +56
# Scope of choices which are not disabled
#
# @api public
def enabled
reject(&:disabled?)
end
Copy link
Owner

Choose a reason for hiding this comment

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

This is a good addition regardless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, will cherry-pick this chunk too 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in #144

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

3 participants