-
Notifications
You must be signed in to change notification settings - Fork 2
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
Dev.ej/wizard navigation #561
Conversation
Using a custom NodeMixinWithNavigation with prev() and next() method will enable going back through the tree during the wizard.
Fixes formatting in many places. Adds nice highlighting in some. But, do not redefine builtin print, use rich_print to be clear everywhere. Fixes #546: question titles being truncated instead of wrapped.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #561 +/- ##
==========================================
+ Coverage 74.87% 76.14% +1.26%
==========================================
Files 46 46
Lines 3144 3374 +230
Branches 510 550 +40
==========================================
+ Hits 2354 2569 +215
- Misses 691 701 +10
- Partials 99 104 +5 ☔ View full report in Codecov by Sentry. |
938ffd4
to
f6a405b
Compare
…nd Step Plus some other tydying of wizard steps and docs
Required changing the concise @contextmanager functions into more verbose classes which can be entered in more than one `with` statement. Less elegant, sigh, but it's what I needed to test back() in a simple way.
10500a0
to
d955aeb
Compare
882f3c7
to
e22f6e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good Eric , nice work! Super practical what you did! I ran a series of various tests, all with success :-)
Some steps have "automatic" logic that depends on some element of context, so let it be calculated in those cases. But beware, this cannot be done in `prompt()`: it must not have any side effects since it is not called in resume mode.
Also adjust unit testing accordingly.
e22f6e8
to
19cfa35
Compare
PR Goal?
Add several navigation features to the EveryVoice new-project wizard:
--trace
to auto display it at each step)--debug-state
displays the current state before each question--resume-from
option to resume from a saved stateFixes?
Fixes #545
Fixes #468
Fixes #546
Feedback sought?
Code review, and playing around with the wizard to see how it feels, how it works, improvements you might like to see.
Priority?
beta
Tests added?
Not yet - I'm making this draft PR to seek feedback but I'll add unit testing before finalizing it.In progress: navigation has unit testing now.
How to test?
Run the wizard with the new options, hit Ctrl-C are various points during the wizard and play with the options.
Confidence?
medium-high, since I have tested it interactively quite extensively, but will be higher once unit testing is in place.
Version change?
might have warranted a minor version bump since it's new features, but we're preparing the next alpha so that doesn't make sense. Nothing broken, in any case, so no major bump.
Related PRs?
nope, it's all in EV itself.