Swapped pyinquirer with questionary #799

KevinMenden · 2020-11-26T12:20:13Z

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

To resolve #726 , swapped PyInquirer with questionary in launch.py and setup.py

Installed locally and nf-core launch seemed to behave as expected.

codecov · 2020-11-26T12:36:54Z

Codecov Report

Merging #799 (b0b407f) into dev (3e0895e) will decrease coverage by 0.09%.
The diff coverage is 32.00%.

@@            Coverage Diff             @@
##              dev     #799      +/-   ##
==========================================
- Coverage   82.97%   82.88%   -0.10%     
==========================================
  Files          13       13              
  Lines        2726     2717       -9     
==========================================
- Hits         2262     2252      -10     
- Misses        464      465       +1

Impacted Files	Coverage Δ
nf_core/launch.py	`70.38% <32.00%> (-0.57%)`	⬇️
nf_core/modules.py	`88.11% <0.00%> (-1.00%)`	⬇️
nf_core/list.py	`80.00% <0.00%> (-0.46%)`	⬇️
nf_core/utils.py	`91.42% <0.00%> (+0.06%)`	⬆️

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 298376d...b0b407f. Read the comment docs.

ewels

Wow, this really was an easy switch! Hardly any code at all. Kudos @KevinMenden for actually sitting down and doing this - great work!

Love that the currently selected option in a Select list has a highlighted background!
~~I notice that we're still shuffling the default option to the top of the list. Does Questionary support creating a list with the pointer at a given default instead of always being on the first option?~~ (Sounds like no?)
Similar: we're still using the if answer == {}: raise KeyboardInterrupt hack to capture ctrl-c events. Can Questionary allow KeyboardInterrupt events to bubbled to our code without capturing them first? If so could remove this hack. (Sounds like yes?)
Something I could never do with PyInquirer: Is it possible to show one thing in a Select list but return a different value?
- What I originally wanted to do was to show the default / filled in value for each option in a group. For example: --my_param [foo]. But this broke PyInquirer, as the select list would only take value so had to have just --my_param to work.
- Would be awesome if we could show the given values for each option in the group list view, bonus points by colouring if different to the schema default.
Select dropdown prompt - can we customise this to get the nice orange arrow back? 🤓 (not a dealbreaker)

Phew, finally managed to break it.. I was starting to get worried. Running nf-core launch atacseq --show-hidden and navigate through to --publish_dir_mode. This option uses enum to limit the allowed return values. Typing in random jibberish gave the following traceback:

│ /opt/miniconda3/envs/py3.8/lib/python3.8/site-packages/questionary/prompts/common.py:448 in create_inquirer_layout                     │
│                                                                                                                                        │
│   445 ) -> Layout:                                                                                                                     │
│   446 │   """Create a layout combining question and inquirer selection."""                                                             │
│   447 │                                                                                                                                │
│ ❱ 448 │   ps = PromptSession(get_prompt_tokens, reserve_space_for_menu=0, **kwargs)                                                    │
│   449 │   _fix_unecessary_blank_lines(ps)                                                                                              │
│   450 │                                                                                                                                │
│   451 │   validation_prompt = PromptSession(bottom_toolbar=lambda: ic.error_message, **kwargs)
╰────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
TypeError: __init__() got an unexpected keyword argument 'validate'

nf_core/launch.py

ewels

Couple of super minor things

nf_core/launch.py

Co-authored-by: Phil Ewels <[email protected]>

KevinMenden · 2020-11-27T07:54:24Z

Thanks for the review Phil and sorry for the typos!

And especially thanks for breaking it :D I'll have a look into that :)

Yes it was hardly any brainwork - really simply swapping the two. Probably possible because the questionary code was based on PyInyquirer to some extent, that's why they are so similar.

No I think specifying a default is not possible currently ... I'll have another look.
Actually yes, looking at the code it seems like they are supported. I will remove our workaround and keyboard-interrupt it - let's see if it works 👍
I see, so you want to have e.g. '--single_end (False)' or something similar to that. That would be quite nice I agree .... will have a look into that as well :)
Haha, okay I'll try to get the orange cursor back 😁

KevinMenden · 2020-11-27T08:58:12Z

Good news, there was a PR 2 months back that introduced the default option in questionary. So we can just comment-out the workaround code and default options are automatically selected.

Questionary also 'catches' keyboard interrupts, but instead of exiting it just returns None, so I fear we'll probably have to stick to the workaround there ....

Bad news is, the error you encountered is caused because in questionary, listor choice questions don't have the validateparameter anymore - for some reason. It's still there for other questions like text, that's why it works for those and we can keep it. Will see if there's another way including the validation in list choices.

KevinMenden · 2020-11-27T09:16:00Z

unsafe_prompt doesn't catch keyboard interrupts, which basically means it behaves as we want it - exiting the program when interrupted. So this should fix this workaround :)

KevinMenden · 2020-11-27T12:19:07Z

Okay so for now I have commented all things related to the validator for 'enum' objects --> I was wondering whether we actually need validation of the input there? Because it is a selection from a list, the user shouldn't have the option to pass wrong inputs - or am I missing something?

Tried to monkey-patch your arrow back in, but it didn't work somehow :/ but at least now it's orange again!
There's also a list for the style at the beginning if you want to adapt that a bit ;)

Didn't find any specific feature that would allow to do what you suggest in point 4. So we'd have to code it ourselves I guess, if that's even possible.

ewels

Quick code review on my phone, will try to test again when I get a chance.

This is looking brilliant - it makes me very happy to see my hacks and workarounds being removed! 😄

.gitignore

CHANGELOG.md

nf_core/launch.py

setup.py

Co-authored-by: Phil Ewels <[email protected]>

* Use ANSI colours instead of hex values, so that colours honour the terminal settings (and match the nf-core logo) * Show the value for each parameter in the select list - either the default from the schema or the entered value

ewels · 2020-12-02T09:58:31Z

Something I could never do with PyInquirer: Is it possible to show one thing in a Select list but return a different value?

Turns out that you can do this with Questionary! It accepts both title and value (see code), which can be different.

I pushed a change to add this above, so now we get either the entered value, or the schema default in square brackets in the list view:

? Nextflow command-line flags  (Use arrow keys)
 » Continue >>
   ---------------
   -name
   -profile
   -work-dir  [./work]
   -resume  [False]

? Input/output options  (Use arrow keys)
 » Continue >>
   ---------------
   input
   single_end  [False]
   fragment_size  [0]
   seq_center
   outdir  [./results]
   email  [[email protected]]

This is super cool! Makes the interface a lot more usable as you can see at a glance that -resume is False by default and so you probably want to change it. Before this, you'd have to go through each option to find out what the current value was.

I'd love to have the default values printed in a different colour, but I couldn't figure out how to do that and have run out of time.

ewels · 2020-12-02T09:59:20Z

Also, I changed the colours - I hope that's ok! Instead of absolute hex codes, I switched to ANSI colour codes. This means that the colours (a) should exactly match the nf-core ANSI logo, which also uses the same codes, and (b) will change according to the colour scheme that people have set on their terminal.

ewels

Looks great now! If you're happy with my changes then please merge 🚀

KevinMenden · 2020-12-02T10:01:42Z

Awesome, I completely oversaw that. That's indeed pretty neat :-)
Thanks!!

ewels · 2020-12-02T10:24:53Z

I'm not sure that it was actually documented it anywhere? I only found it when digging around in the questionary code base..

KevinMenden · 2020-12-02T10:28:16Z

Yes same for the other things - they could certainly use a better documentation ... but I understand that it's not what developers like to do :)
There's even an open issue for it
tmbo/questionary#47

Swapped pyinquirer with questionary

378a688

KevinMenden requested a review from ewels November 26, 2020 12:20

KevinMenden added 2 commits November 26, 2020 13:30

fixed test_launch.py, updated changelog

b962262

fixed typo

78ed52a

added blank lines to changelog

b9116e9

ewels reviewed Nov 26, 2020

View reviewed changes

nf_core/launch.py Outdated Show resolved Hide resolved

ewels reviewed Nov 26, 2020

View reviewed changes

nf_core/launch.py Outdated Show resolved Hide resolved

nf_core/launch.py Outdated Show resolved Hide resolved

typo fixes

7919bd2

Co-authored-by: Phil Ewels <[email protected]>

fixed bug with new questionare_objects name

c97d066

KevinMenden added 3 commits November 27, 2020 10:44

catch keyboard interrupt, default list choice

e6f8715

uncommented 'validate' tests in test_launch

92eae6d

Updated style; fixed unsafe_promt in test

e12ab1c

KevinMenden added 2 commits November 27, 2020 15:44

Deleted validation for 'enum' objects

836814e

made black happy again

4bdab7c

ewels requested changes Nov 27, 2020

View reviewed changes

KevinMenden and others added 5 commits November 29, 2020 08:48

Update .gitignore

01be190

Co-authored-by: Phil Ewels <[email protected]>

Update .gitignore

424683c

Co-authored-by: Phil Ewels <[email protected]>

remove questionary.constants import

45b28fb

Co-authored-by: Phil Ewels <[email protected]>

Update CHANGELOG.md

3099aa7

Co-authored-by: Phil Ewels <[email protected]>

Specified questionary version, added prompt_toolkit

2a35e58

KevinMenden requested a review from ewels December 1, 2020 11:24

ewels added 2 commits December 2, 2020 10:51

Use ANSI colours, show value in select list

622a557

* Use ANSI colours instead of hex values, so that colours honour the terminal settings (and match the nf-core logo) * Show the value for each parameter in the select list - either the default from the schema or the entered value

Fix changelog link

b0b407f

ewels approved these changes Dec 2, 2020

View reviewed changes

KevinMenden merged commit ae2d7e7 into nf-core:dev Dec 2, 2020

KevinMenden mentioned this pull request Dec 8, 2020

PyInquirer conda conflict for nf-core 1.10.2 and python 3.7 #726

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swapped pyinquirer with questionary #799

Swapped pyinquirer with questionary #799

KevinMenden commented Nov 26, 2020 •

edited

Loading

codecov bot commented Nov 26, 2020 •

edited

Loading

ewels left a comment

ewels left a comment

KevinMenden commented Nov 27, 2020

KevinMenden commented Nov 27, 2020

KevinMenden commented Nov 27, 2020

KevinMenden commented Nov 27, 2020

ewels left a comment

ewels commented Dec 2, 2020

ewels commented Dec 2, 2020

ewels left a comment

KevinMenden commented Dec 2, 2020

ewels commented Dec 2, 2020

KevinMenden commented Dec 2, 2020

Swapped pyinquirer with questionary #799

Swapped pyinquirer with questionary #799

Conversation

KevinMenden commented Nov 26, 2020 • edited Loading

PR checklist

codecov bot commented Nov 26, 2020 • edited Loading

Codecov Report

ewels left a comment

Choose a reason for hiding this comment

ewels left a comment

Choose a reason for hiding this comment

KevinMenden commented Nov 27, 2020

KevinMenden commented Nov 27, 2020

KevinMenden commented Nov 27, 2020

KevinMenden commented Nov 27, 2020

ewels left a comment

Choose a reason for hiding this comment

ewels commented Dec 2, 2020

ewels commented Dec 2, 2020

ewels left a comment

Choose a reason for hiding this comment

KevinMenden commented Dec 2, 2020

ewels commented Dec 2, 2020

KevinMenden commented Dec 2, 2020

KevinMenden commented Nov 26, 2020 •

edited

Loading

codecov bot commented Nov 26, 2020 •

edited

Loading