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.

Sign up for GitHub

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

Jump to bottom

Better UI for command line launch #829

Merged
merged 6 commits into from
Jan 15, 2021
Merged

Better UI for command line launch #829

merged 6 commits into from
Jan 15, 2021

Conversation

ewels
Copy link
Member

@ewels ewels commented Jan 14, 2021 •
edited
Loading

Refined UI around the Questionary prompts:

  • Fixed bug where you couldn't delete something you'd entered
    • Empty answers were ignored, so if you typed something then went back and deleted it then the interface would revert to whatever you typed last - you couldn't reset it to nothing. Now fixed.
  • Group select lists
    • Removed duplicated group heading
    • Stronger highlighting of group title
    • Default values of options formatted with dim text
    • Inputted values of options formatted with yellow text
    • Required options shown with hint if not yet set
    • Errors shown at top of list (as disabled options) if Continue selected with required options not set
  • Single parameters
    • Removed duplicated parameter title
    • Stronger highlighting of parameter title

Screenshots of cli interface before and after changes in this PR:

BeforeAfter

image

image

image

image

Implementation of #816 and with help from tmbo/questionary#107

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

ewels added 4 commits January 14, 2021 16:30
@ewels
Launch - rich formatting for prompts, show required.
51fc2df 
* Use ansi colours for the default values and filled values in the group select view of params (nf-core#816)
* Add to group select prompt a warning that a value is required, if not yet filled and no default
@ewels
Launch: Refine rich text output for cli prompts
29108ff
@ewels
Launch: No duplication for parameter heading. Fix order of code for g…
48b81e0 
…roup heading.
@ewels
Launch cli: Allow answers to be deleted
ee7e971
@ewels ewels added the command line tools Anything to do with the cli interfaces label Jan 14, 2021
@ewels ewels requested review from mashehu, KevinMenden and a team January 14, 2021 18:03
@codecov
Copy link

codecov bot commented Jan 14, 2021 •
edited
Loading

Codecov Report

Merging #829 (32a1790) into dev (bb5c10d) will decrease coverage by 0.34%.
The diff coverage is 8.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #829      +/-   ##
==========================================
- Coverage   78.34%   78.00%   -0.35%     
==========================================
  Files          22       22              
  Lines        2480     2491      +11     
==========================================
  Hits         1943     1943              
- Misses        537      548      +11
Impacted Files Coverage Δ
nf_core/launch.py 67.03% <8.00%> (-2.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb5c10d...32a1790. Read the comment docs.

KevinMenden
KevinMenden approved these changes Jan 15, 2021
View reviewed changes
Copy link
Contributor

@KevinMenden KevinMenden left a comment

Choose a reason for hiding this comment

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

Very nice, couldn't find anything suspicious 👍

nf_core/launch.py
Comment on lines +456 to +463
# Show error messages if we have any
for msg in error_msgs:
question["choices"].append(
questionary.Choice(
[("bg:ansiblack fg:ansired bold", " error "), ("fg:ansired", f" - {msg}")], disabled=True
)
)
error_msgs = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't that make more sense after line 491, where you check for errors?
Or does it require a 'fresh' question object?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the logic is a bit funny here. If there are errors then the question is asked again. So in lines 491 we collect error messages, then the loop fires again. Here we are printing the errors if there are any from the last time we went around and then clearing them once shown (so that they don't persist forever).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I get it, makes sense like that :D thanks!

@ewels ewels merged commit ef3f73a into nf-core:dev Jan 15, 2021
@ewels ewels deleted the questionary-prompt-formatting branch January 15, 2021 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KevinMenden KevinMenden KevinMenden approved these changes

@mashehu mashehu Awaiting requested review from mashehu

Assignees
No one assigned
Labels
command line tools Anything to do with the cli interfaces
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ewels @KevinMenden