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

MultiList: validate both min and max on keyenter #173

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sveinn
Copy link

@sveinn sveinn commented Aug 4, 2021

Describe the change

Fixes issue 172

Requirements

  • Tests written & passing locally?
  • Code style checked?
  • Rebased with master branch?
  • Documentation updated? (no, but checking this since no documentation change necessary)
  • Changelog updated?

@sveinn sveinn changed the title Issue 172: MultiList: validate both min and max on keyenter MultiList: validate both min and max on keyenter Aug 4, 2021
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.

Thank you for submitting this PR.

As things stand I'm unable to accept it. At a minimum, there needs to be a test case that proves the issue has been fixed. Also, I'd like you to revert any formatting changes to the code that isn't affected directly by the issue. My preference is for PR to be focused on what is being fixed/added. I'd also appreciate an entry in the changelog.

lib/tty/prompt/multi_list.rb Outdated Show resolved Hide resolved
lib/tty/prompt/multi_list.rb Outdated Show resolved Hide resolved
@sveinn sveinn force-pushed the issue-172 branch 2 times, most recently from da54e43 to 576ff32 Compare August 20, 2021 22:09
@sveinn
Copy link
Author

sveinn commented Aug 21, 2021

Thank you for submitting this PR.

As things stand I'm unable to accept it. At a minimum, there needs to be a test case that proves the issue has been fixed. Also, I'd like you to revert any formatting changes to the code that isn't affected directly by the issue. My preference is for PR to be focused on what is being fixed/added. I'd also appreciate an entry in the changelog.

My pleasure. I've addressed your requests, please let me know if I missed anything. Thanks!

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.

Thank you for working on the changes. It seems you have missed out on a variable that affects some tests. I left few extra comments to keep the tests focused. Whilst reviewing I noticed that it would be good to consider the situation when a user provides min greater than max. Should we check for this and raise an appropriate and meaningful error?

lib/tty/prompt/multi_list.rb Outdated Show resolved Hide resolved
spec/unit/multi_select_spec.rb Show resolved Hide resolved
spec/unit/multi_select_spec.rb Show resolved Hide resolved
@sveinn sveinn force-pushed the issue-172 branch 2 times, most recently from 5e5fa29 to 62c6f40 Compare August 22, 2021 05:15
@sveinn
Copy link
Author

sveinn commented Aug 22, 2021

Thank you for working on the changes. It seems you have missed out on a variable that affects some tests. I left few extra comments to keep the tests focused. Whilst reviewing I noticed that it would be good to consider the situation when a user provides min greater than max. Should we check for this and raise an appropriate and meaningful error?

Woops, sorry about that.

Should we check for this and raise an appropriate and meaningful error?

Good idea. Made a stab at this, let me know what you think.

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.

Thanks for addressing the suggestions so quickly. We're nearly there. I left few more comments on the min and max checking as well as tests formatting.

spec/unit/multi_select_spec.rb Outdated Show resolved Hide resolved
spec/unit/multi_select_spec.rb Outdated Show resolved Hide resolved
spec/unit/multi_select_spec.rb Outdated Show resolved Hide resolved
spec/unit/multi_select_spec.rb Outdated Show resolved Hide resolved
spec/unit/multi_select_spec.rb Outdated Show resolved Hide resolved
spec/unit/multi_select_spec.rb Outdated Show resolved Hide resolved
lib/tty/prompt/multi_list.rb Outdated Show resolved Hide resolved
@sveinn
Copy link
Author

sveinn commented Aug 23, 2021

Thanks for addressing the suggestions so quickly. We're nearly there. I left few more comments on the min and max checking as well as tests formatting.

Sure thing. Just addressed your comments. Lemme know if there are any more, or if I missed something. Thanks!

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.

Things are looking good, I left few minor comments.

spec/unit/multi_select_spec.rb Show resolved Hide resolved
spec/unit/multi_select_spec.rb Show resolved Hide resolved
lib/tty/prompt/multi_list.rb Outdated Show resolved Hide resolved
lib/tty/prompt/multi_list.rb Show resolved Hide resolved
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.

Thank you for such a quick turnaround. I left a comment on the changelog description. One thing to make you aware of is that I'm planning to merge another PR before yours as it was long in waiting and it is a significant change. There shouldn't be any conflicts with your work but you'll need to rebase.

spec/unit/multi_select_spec.rb Show resolved Hide resolved
CHANGELOG.md Outdated
Comment on lines 6 to 7
* Fix MultiList#keyenter validation - min option also validated now when both min and max are specified.
* Fix MultiList#keyenter validation - ensure min is not greater than max
Copy link
Owner

Choose a reason for hiding this comment

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

I'd consider rewording without pointing to any particular method. The validation also ensures that you cannot select more than the allowed number of choices before confirming your selection. The second point arguably could be thought of as change rather than a fix and recorded as such?

Copy link
Author

Choose a reason for hiding this comment

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

I'd consider rewording without pointing to any particular method.

Makes sense. How about we change it to multi_list since that's what the user interacts with?

The validation also ensures that you cannot select more than the allowed number of choices before confirming your selection.

Yes. I didn't call this out in the changelog since this was already in working order prior to my fix. Would you like me to mention it though?

The second point arguably could be thought of as change rather than a fix and recorded as such?

Good point.

What are your thoughts on this:

### Fixed
* Fix multi_select validation - min option also validated now when both min and max are specified
### Changed
* Change multi_select validation to ensure min is not greater than max

Copy link
Author

@sveinn sveinn Sep 25, 2021

Choose a reason for hiding this comment

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

Made a change and re-requested a review. (reads multi select instead of multi_select since you had suggested to not refer to a particular method)

@Thomascountz
Copy link

👋🏾 Hi there, is there any additional help needed on this PR? I was just about to open a new one, but I'd love to help get this one merged instead 😃

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.

multi_select validation for "min" option ignored if "max" validation passes
3 participants