Skip to content

consolidate y/n prompt in shell#3684

Merged
ctubbsii merged 8 commits intoapache:2.1from
EdColeman:shell_standardize_yes_no
Aug 14, 2023
Merged

consolidate y/n prompt in shell#3684
ctubbsii merged 8 commits intoapache:2.1from
EdColeman:shell_standardize_yes_no

Conversation

@EdColeman
Copy link
Contributor

@EdColeman EdColeman commented Aug 10, 2023 •
edited
Loading

Prompting the user for y/n was handled in multiple places in shell commands. This PR consolidates those into a single method in the Shell.

  • Also includes some check style errors that we being flagged.

This was identified as a follow-on issue in PR #3680

@EdColeman EdColeman mentioned this pull request Aug 10, 2023
Merged
@EdColeman EdColeman requested a review from ctubbsii August 10, 2023 20:55
@EdColeman EdColeman self-assigned this Aug 10, 2023
@EdColeman
Copy link
Contributor Author

EdColeman commented Aug 10, 2023

There is one usage in DeleteFormatter that is prompting y/n but seems to be following a different pattern was left alone for now and is not part of this PR.

@EdColeman EdColeman marked this pull request as draft August 11, 2023 00:50
@EdColeman
Copy link
Contributor Author

EdColeman commented Aug 11, 2023

Set this a draft - there may be an issue in ShellIT - this should not be merged until that is resolved

@ctubbsii
Copy link
Member

ctubbsii commented Aug 11, 2023

There is one usage in DeleteFormatter that is prompting y/n but seems to be following a different pattern was left alone for now and is not part of this PR.

I think the main thing about that one is that it catches EndOfFileException, which I think handles things like Ctrl-C, and aborts (like returning false). That could be baked into the behavior of the consolidated implementation.

ctubbsii
ctubbsii requested changes Aug 11, 2023
View reviewed changes
@ddanielr
Merge branch '2.1' into shell_standardize_yes_no
4254f1f
@ddanielr
Fixes ShellIT and implements PR feedback
ea20d04 
* ShellIT was erroring on the configTest method.
  Adds force handler logic to pass test

* Renames `yorn` method to `confirm` for better readability.

* Simplifies logic in `DeleteNamespaceCommand` and `DropUserCommand`.

* Updates wording in `MergeCommand` to convey more concern.
@ddanielr
Copy link
Contributor

ddanielr commented Aug 13, 2023

Set this a draft - there may be an issue in ShellIT - this should not be merged until that is resolved

Found the issue in ShellIT was due to the forceOpt handling in the ConfigCommand

Issue could be replicated by checking out 5ac8ac and running the following test

mvn clean verify -Dit.test=org.apache.accumulo.test.shell.ShellIT#configTest -Dtest=foo -Dspotbugs.skip

This issue has been fixed in ea20d0

@ddanielr ddanielr marked this pull request as ready for review August 13, 2023 03:56
DomGarguilo
DomGarguilo approved these changes Aug 13, 2023
View reviewed changes
Copy link
Member

@DomGarguilo DomGarguilo left a comment

Choose a reason for hiding this comment

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

LGTM. Tested out the changed commands and everything seems to be working as expected.

DomGarguilo
DomGarguilo approved these changes Aug 13, 2023
View reviewed changes
Copy link
Member

@DomGarguilo DomGarguilo left a comment

Choose a reason for hiding this comment

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

LGTM. Tested out the changed commands and everything seems to be working as expected.

ctubbsii
ctubbsii requested changes Aug 14, 2023
View reviewed changes
Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

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

I think it's worth considering restoring the slight change in behavior that existed previously that used the more flag to discontinue processing the rest of the items in the loop, before this is merged. Perhaps my suggestion to use Optional for this will work, or perhaps there's something cleaner. But, I don't think it's a good idea to force users to continue to be prompted for the remaining items in a loop if the previous code aborted the loop.

ddanielr and others added 3 commits August 14, 2023 17:51
@ddanielr
Changes confirm return type to Optional<Boolean>
fde3ae3 
Replaces the original boolean return with Optional<Boolean> to handle
abort actions.

Standardizes the yes/no prompt in DeleterFormatter.
@ctubbsii
Merge branch '2.1' into shell_standardize_yes_no
4111014
@ctubbsii
Code review improvements
160ee77 
* Replace `.filter(y -> y).isPresent()` with `.orElse(false)`
* Simplify TableOperation loop logic
ctubbsii
ctubbsii approved these changes Aug 14, 2023
View reviewed changes
Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

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

LGTM now

@ctubbsii ctubbsii requested a review from ddanielr August 14, 2023 19:48
EdColeman
EdColeman commented Aug 14, 2023
View reviewed changes
Copy link
Contributor Author

@EdColeman EdColeman left a comment

Choose a reason for hiding this comment

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

LGTM

ddanielr
ddanielr approved these changes Aug 14, 2023
View reviewed changes
Copy link
Contributor

@ddanielr ddanielr left a comment

Choose a reason for hiding this comment

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

Tested these changes with table deletion commands and you can now exit out of the loop of multi-table operations instead of getting prompted for each one.

@ctubbsii ctubbsii merged commit 38c261d into apache:2.1 Aug 14, 2023
@EdColeman EdColeman deleted the shell_standardize_yes_no branch September 19, 2023 17:23
@ctubbsii ctubbsii modified the milestones: 3.1.0, 2.1.3 Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ctubbsii ctubbsii ctubbsii approved these changes

@ddanielr ddanielr ddanielr approved these changes

@DomGarguilo DomGarguilo DomGarguilo approved these changes

Assignees

@EdColeman EdColeman

Labels

None yet

Projects

None yet

Milestone

2.1.3

Development

Successfully merging this pull request may close these issues.

4 participants

@EdColeman @ctubbsii @ddanielr @DomGarguilo