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

Adds the description and show_description options to Choice and InquirerControl #330

Merged
merged 9 commits into from
Jan 11, 2024

Conversation

viniciusdc
Copy link
Contributor

@viniciusdc viniciusdc commented Oct 19, 2023

What is the problem that this PR addresses?

closes #269
...

How did you solve it?

I am including a new show_description boolean value into the InquirerControl class to allow a new choice attribute called description to be displayed at the bottom of the terminal window -- Similar execution as the current show_selected attribute.

More details in the linked issue.
...

Checklist

  • I have read the Contributor's Guide.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@viniciusdc
Copy link
Contributor Author

Hi @pmeier, I know you were also interested in this addition. Feel free to push and update the PR as needed.

@viniciusdc
Copy link
Contributor Author

Hi @kiancross, sorry for the delay with the PR. Would you mind approving the CI tests to run? Thanks in advance 😄

Copy link
Contributor

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

@kiancross LMK if you want more tests and if yes what exactly.

@@ -38,6 +38,7 @@ def checkbox(
use_jk_keys: bool = True,
use_emacs_keys: bool = True,
instruction: Optional[str] = None,
show_description: bool = True,
Copy link
Contributor

Choose a reason for hiding this comment

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

@viniciusdc I've changed this to True by default, because that is what the user expects when they set a description on a Choice. Plus, since there are no descriptions yet, previous code is not affected by this.

Comment on lines +230 to +231
show_selected=True,
show_description=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Having both active asserts that they can coexist.

weiduhuo added a commit to weiduhuo/StarRailAssistant that referenced this pull request Oct 29, 2023
@viniciusdc
Copy link
Contributor Author

viniciusdc commented Dec 21, 2023

Hi @kiancross, sorry for the constant pings. I just wanted to know if you could have a look into this whenever you have some time 😄 Happy holidays 🎄

Copy link
Collaborator

@kiancross kiancross left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for the PR. Can you force-push the last commit to re-trigger the GitHub actions?

@pmeier
Copy link
Contributor

pmeier commented Dec 29, 2023

@kiancross Maintainer action is required to run CI:

image

@kiancross
Copy link
Collaborator

kiancross commented Dec 29, 2023

CI is running now. But some of the tests are failing.

Comment on lines 449 to 450
else:
tokens.pop() # Remove last newline.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
else:
tokens.pop() # Remove last newline.
if not self.show_description and not self.show_selected:
tokens.pop() # Remove last newline.

I think something like this will probably fix the failing tests.

@pmeier
Copy link
Contributor

pmeier commented Jan 2, 2024

@kiancross could you approve CI again and take another look? Locally, everything is passing for me after 9305d57.

@pmeier
Copy link
Contributor

pmeier commented Jan 8, 2024

@kiancross Anything left to do now that CI is passing or can this be merged?

@kiancross kiancross merged commit bba92b4 into tmbo:master Jan 11, 2024
30 checks passed
@kiancross
Copy link
Collaborator

Merged! Thanks for your work on this :)

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.

[ENH] Include show_description to InquirerControl
3 participants