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

Add the ability to customise how questions are asked #674

Closed
pfmoore opened this issue May 23, 2022 Discussed in #673 · 22 comments · Fixed by #689
Closed

Add the ability to customise how questions are asked #674

pfmoore opened this issue May 23, 2022 Discussed in #673 · 22 comments · Fixed by #689
Labels
enhancement good first issue Easy things for newbies help wanted The issue is valid, but we need community contributions to fix it

Comments

@pfmoore
Copy link
Contributor

pfmoore commented May 23, 2022

Discussed in #673

Originally posted by pfmoore May 23, 2022
The prompt currently appears to be a 🎤emoji, followed by the help (if specified), then the item name and a format prompt. I'd like to be able to change the prompt as I find it difficult to read (in particular, the format prompt is very distracting, and I'd like to omit it). Is that possible?

Having checked the source code, it seems that this isn't possible. But the questionary library looks very flexible, so it would be possible to do this if there was a way to describe the format in the template configuration. Would such a change be welcome? I'd be more than happy to work on a PR for this, but I'm very new to the project (I'm currently trying to migrate my existing cookiecutter templates over to copier) so I don't know if that's something that would be considered a reasonable feature for the project.

I'd imagine doing this via settings in the copier.yml file, so the format would be decided by the template, rather than by the user. Would that make sense?

@yajo
Copy link
Member

yajo commented May 23, 2022

Yes, this seems reasonable. However I'd like to know beforehand what's your suggestion for a less distracting proposal. Maybe we can just hardcode something simpler, as most people won't complain about simplicity.

@yajo yajo added enhancement good first issue Easy things for newbies help wanted The issue is valid, but we need community contributions to fix it labels May 23, 2022
@pfmoore
Copy link
Contributor Author

pfmoore commented May 23, 2022

My main concerns are:

  1. I don't find the "Format: {type}" section of the prompt useful, and the fact that it isn't separated from the value by punctuation is very distracting.
  2. The way the emoji is on the help line if there's help, but on the variable name line if there's a variable, is inconsistent and makes it hard for me to review what I entered.
  3. It's not at all obvious (to me, at least) how to word the help - as a question, or a description.

I think I'd prefer something like:

project_name:
    type: str
    help: Enter your project name
repeat:
    type: int

giving

🎤 Enter your project name: my_project
🎤 repeat: 12

So basically, "{emoji} {help_or_var_name}: "

If you wanted to include the type, maybe add " [{type}]" just before the colon, but only when it's not "str", so you would have:

🎤 Enter your project name: my_project
🎤 repeat [int]: 12

The emoji is OK, but it's a bit of a personal preference. If copier had a "user settings" file, I'd suggest letting the user alter the emoji in that, but I don't think it's something you'd want on a command line. So maybe don't worry about it for now.

@yajo
Copy link
Member

yajo commented May 23, 2022

Seems good to me. Would you want to volunteer that PR?

@pfmoore
Copy link
Contributor Author

pfmoore commented May 23, 2022

Absolutely, I'd love to 🙂

@pfmoore
Copy link
Contributor Author

pfmoore commented May 25, 2022

I've reviewed the copier templates under the copier-template topic. There seem to be three common styles of help:

  1. A simple description, with no puntuation. ("Your user name")
  2. A one-line question. ("Do you want to include tests?")
  3. A multi-line explanation.

In order to avoid breaking people's existing templates, I propose that for case (1) we use the help with a ": " suffix, in case (2) we use the help unchanged. I have yet to see other forms such as "A description with punctuation!" and I'm not entirely sure what to do with them. For now, I'll defer the problem and just treat them like case (1).

For case (3), I think that applying basically the same rule (remove any trailing newlines, then either add a space or ": " depending on where there's a question mark) gives something fairly OK. But I'm a little hesitant here, as the template developers have obviously put some effort into writing useful help, and I don't want to mess that up.

What do you think?

Also, I have a problem in that a bunch of the tests fail on Windows, and pre-commit complains (the prettier and mixed line ending checks fail, but the diff shows nothing but "LF will be replaced by CRLF" and a couple of "No newline at end of file" changes). I'm not at all sure I know what to do to fix this, and in any case I don't want to mix up this change with a bunch of "try to make things work on Windows" changes.

I can switch off the pre-commit hook in order to actually commit my code, but the tests are more of an issue. There seem to be a lot of tests that depend on the question format, and they all use pexpect, which doesn't work on Windows so those tests are set to xfail. I'll see if I can get a Linux development environment set up using WSL, but otherwise I may be able to do the code change, but not fix the existing tests that rely on the existing prompt layout.

@pfmoore
Copy link
Contributor Author

pfmoore commented May 25, 2022

Actually, a much simpler approach here might be to just add a new attribute for a question, "prompt". If the prompt is set, it is used in place of the string f"{self.var_name}? Format: {self.get_type_name()}". This means that the default behaviour remains the same, but template authors can use a different prompt if they want.

Would that be acceptable? I've just updated a draft PR (#680) which adds this functionality. No docs or tests yet, I mainly just want to see if my change passes the existing test suite at this point.

@yajo
Copy link
Member

yajo commented May 25, 2022

I don't think that adding prompt is very useful, as it would be pretty similar to just adding another line to help.

Regarding all those use cases, the most complex ones are multiline string inputs, which in turn can be formatted and validated as json or yaml inputs.

One option I think would be quite good would be to always put a newline before the answer. So, questions template would be something like {help}\n> and look like:

Enter your project name
> (user writes here)

If the template author wants to finish with colon, dot, question mark... it's up to them.

Also we have selection and boolean questions, and possibly some other...

Regarding tests, setting up a WSL dev env should be pretty straightforward. Sadly, Windows is a 2nd class citizen here (I just rely on CI for it). Taking care the pexpect tests would be quite important for this feature to land, I'm afraid. Those took a lot of work to do!

@yajo
Copy link
Member

yajo commented May 26, 2022

I'll see if I can get a Linux development environment set up using WSL

Oh BTW I just remembered we support Gitpod. Check https://copier.readthedocs.io/en/stable/contributing/#dev-environment-setup

@pfmoore
Copy link
Contributor Author

pfmoore commented May 26, 2022

OK, I've got WSL set up. What I'm trying to do at the moment is refactor the tests so that rather than every test depending on the exact form of the prompt, there's a single helper that "expects" a prompt. That will make changing the prompt much less messy.

But I'm hitting an issue with test_complex_questions:test_cli_interactive. The current code contains blocks like

    q = ["Wanna give me any more info?", "anything_else", "Format: yaml"]
    tui.expect_exact(q)
    tui.sendline("- Want some grog?")
    tui.expect_exact(invalid + q)
    tui.sendline("- I'd love it")
    tui.send(Keyboard.Esc + Keyboard.Enter)

If I'm reading the documentation of expect_exact correctly, passing a list checks if any one of them matches ("If you pass a list of patterns and more than one matches, the first match in the stream is chosen"). But the code above looks like it's intending to test that the whole list is displayed.

I'm certainly not sure enough of how pexpect works to be completely clear on what's happening, but if I'm reading things correctly, the existing code does the following:

  1. Check for one of "Wanna give me any more info?", "anything_else", "Format: yaml"
  2. Find the first.
  3. Send "- Want some grog?". No problem here, questionary will still be waiting for further input as it's a multiline field.
  4. Look for one of "Invalid input", "Wanna give me any more info?", "anything_else", "Format: yaml"
  5. Probably(?) find "anything_else", as it hadn't scanned past that yet.
  6. Decide everything is OK and proceed to enter the next line of input and carry on.

So the test passes, but it never actually ensures the "Invalid input" message is displayed. And of course, it shouldn't (because that message isn't displayed) but as written the code seems like it intends to do that check. And the previous checks for "Invalid input" work roughly the same, but in those cases it should be checking that "Invalid input" was displayed, but it isn't actually doing that check...

I've fixed the "anything_else" test as part of my refactoring so that it no longer tries to check for "Invalid input", and my helper which checks for the invalid input message does actually make that check (I know precisely because it failed on "anything_else" 🙂). So hopefully the refactoring adds some value beyond just making it easier to modify the prompt.

By the way, while looking at this, it occurred to me that most of these tests using pexpect, are in practice just testing that the questionary library does what it's documented as doing. While it's certainly reasonable to want to ensure that the UI looks the way you intend it to, I wonder whether it would be possible to add some tests (which would be supported on Windows, unlike the pexpect tests) that worked by mocking questionary and testing it was called with the right inputs? That would give better test coverage on Windows, while still having the full check that the UI looks as expected via the Unix CI. That's probably a lot of work, and I'm not sure I could commit to it right now, but would it be of interest if I did find the time at some point?

@pfmoore
Copy link
Contributor Author

pfmoore commented May 26, 2022

I've submitted #682 for the refactoring of the tests. If that is acceptable, I can then start on changing the format, and we can see what works best (on reflection, {help}\n> isn't good if there's no help, as it gives no indication what the prompt is asking for!)

@yajo
Copy link
Member

yajo commented May 26, 2022

the code above looks like it's intending to test that the whole list is displayed.

Ah! 🤦🏼‍♂️

I simply misunderstood what expect_exact was doing. I fixed that at 5411276 but it seems I forgot to fix the test you're finding now.

(on reflection, {help}\n> isn't good if there's no help, as it gives no indication what the prompt is asking for!)

Indeed! It should give some fallback for those cases.

@yajo
Copy link
Member

yajo commented May 26, 2022

By the way, while looking at this, it occurred to me that most of these tests using pexpect, are in practice just testing that the questionary library does what it's documented as doing. While it's certainly reasonable to want to ensure that the UI looks the way you intend it to, I wonder whether it would be possible to add some tests (which would be supported on Windows, unlike the pexpect tests) that worked by mocking questionary and testing it was called with the right inputs? That would give better test coverage on Windows, while still having the full check that the UI looks as expected via the Unix CI. That's probably a lot of work, and I'm not sure I could commit to it right now, but would it be of interest if I did find the time at some point?

Regarding this point, I'm not sure that would improve the situation here.

The main reason why I decided the current approach is that I wanted to be able to merge blindly dependabot updates, in the case it's updating the TUI-related tests.

Also it's easier when doing TDD for the TUI to expect how it should look like and, then, just do the code.

About windows coverage, there's https://github.com/raczben/wexpect which should be able to let us abstract the OS and run the "same" TUI tests. And in any case, our coverage report aggregates coverage results from all systems.

So yes, windows situation is not the best, but as you said this change would be a ton of work and I'm not sure it'd be worth it. I think that adding support for wexpect would be much more simple and worth it.

@pfmoore
Copy link
Contributor Author

pfmoore commented May 26, 2022

Fantastic. I'd love to help improve Windows support, but I'm cautious of over-committing myself at this point. If there's anything where you feel I could help, though, feel free to ping me 🙂

@yajo
Copy link
Member

yajo commented May 28, 2022

For now the best would be to just try to conditionally use pexpext or wexpect on the TUI tests depending on the OS, and remove the windows xfails.

wexpect was recently refactored. Some time ago I tried that and it didn't work, but it might work today.

Being more used to windows 🪟 than me, I guess you'll be able to do that more easily.

However this is a bit away from this issue's subject so I suggest to keep this thread focused on the prompt stuff and maybe we can open another for the windows coverage problem.

@pfmoore
Copy link
Contributor Author

pfmoore commented May 28, 2022

Getting back on topic, the key point for me with the prompt is that I find the "Format: str" hint extremely distracting. There are a couple of reasons for this:

  1. It's not separated from what the user is typing by punctuation.
  2. It's quite verbose, given the amount of information it adds ("Format: str" in particular adds basically nothing).

Apart from that, my main other preference is that I'd like to be able to construct a template so that each question only takes up a single line, so the prompting is compact. But given that I've seen help texts in the form "What is your name?" and "Enter your name", it's hard to come up with a way that produces a single line prompt that looks good in all cases (given that "what looks good" is extremely subjective here...)

So my suggestion would be:

  1. If the help is a single line, use it as it stands.
  2. If the help is multi-line, use "{help}\n>".
  3. If there's no help, use "{name} ({type})>" but omit the type if it's "(str)".

This seems reasonable to me, with the only odd case being that if people use a 1-line help, they won't get ">" unless they include it in the help.

The alternative would be

  1. If there's help, use "{help}\n>"
  2. Otherwise, use "{name} ({type})\n>" but omit the type if it's "(str)".

This style doesn't give you any way to get a single-line question.

Having played with both of these, I find that I like the first one far better. The single line question style just looks much more readable to me.

There's still some style choices I'm not sure about (> versus some other character, plus the question of whether to try to intelligently adapt if the help text ends with punctuation) but we can work out those details. What's your view on the basic question of whether it's worth the more complex rule of the first approach in order to allow the single-line style?

(By the way, I should say that I've found a gross hack to let a Jinja extension monkeypatch the get_message method, so that even if we can't come to an agreement on a good style here, it's still possible to customise things on a per-template basis. But it's not something I'd want to encourage 🙂)

@yajo
Copy link
Member

yajo commented May 28, 2022

I prefer to add the \n> ending always if you don't mind. I think it lets you see answers properly aligned. Maybe it's just personal taste, but there are more things that short str answers! We have choices, booleans, multiline...

Try with a questionary like this:

# copier.yaml
your_choice:
  type: str
  multiline: true
  help: Choose something
  choices:
    - a
    - b

name:
  type: str
  help: your name, please

some_list:
  type: yaml
  multiline: true
  help: |
    I need you to pass me a list.

    Remember: must be a good list

yn:
  type: bool
  help: want to drink something?

This is how it looks right now, FTR:

Kooha-05-28-2022-12-56-10.mp4

IMHO it would look quite nice if all questions looked pretty much the same, no matter the type of question or the length or lines of text involved in the help attribute.

Maybe you can do a quick mockup and send a video like this to see how this questionary looks with your suggestion?

@pfmoore
Copy link
Contributor Author

pfmoore commented May 28, 2022

Here's my first option (single line when possible):

Option.1.mp4

Here's my second option (always \n>):

Option.2.mp4

@yajo
Copy link
Member

yajo commented May 31, 2022

In my personal taste, I like more the 2nd option. Although after seeing it in action, I think I'd just replace that > character with 2 spaces. It doesn't add much value, and this way the answer would start just below the question (excluding the 🎤 icon). That'd be clean and consistent.

@pawamoy what do you thinhk?

@pawamoy
Copy link
Contributor

pawamoy commented May 31, 2022

I like compact stuff. When all questions are single-line, I prefer the compact version. But since questions (and answers!) can be multiline, I think the second version would be better to handle all cases. It also has a clear benefit: the input field has more width. Not sure it's still accurate (maybe questionary handles it fine), but in previous versions, and just like in most shells, if you go past the right threshold, and therefore on a new line, you can't go back to edit (left/up). On mobile right now, will try to expand later.

@pfmoore
Copy link
Contributor Author

pfmoore commented May 31, 2022

I agree there's no ideal answer that makes every possible question look good. That's why I'd originally thought of letting the config file provide a prompt format that works well with the help texts the config uses. But as we don't want to do that, I can see the argument for having something that's consistent, even if it's not particularly compact.

I'm happy to go with whatever you prefer at this point. For my personal templates, if I want something compact, I can always use the hack I mentioned above...

@yajo
Copy link
Member

yajo commented Jun 1, 2022

OK then let's advance with the double line proposal.

I'd originally thought of letting the config file provide a prompt format that works well with the help texts the config uses. But as we don't want to do that, I can see the argument for having something that's consistent, even if it's not particularly compact.

FWIW in case we wanted to have this configurable, I think it should be configurable per user, not per template. Because it depends more on personal taste, not something the template dev should care about IMHO. The same might happen with colors, fonts...

Also some other experience I had in mind was a fullscreen TUI with backwards and forward buttons to let you edit previous answers. But that's probably something I'll never have time to do 😆.

@pfmoore
Copy link
Contributor Author

pfmoore commented Jun 1, 2022

FWIW in case we wanted to have this configurable, I think it should be configurable per user, not per template.

Possibly. But as we've seen, any given format typically works best with a particular style of help, so there's an argument that it should be tied to the template as well. But as you say, that's something to worry about another day 🙂

Also some other experience I had in mind was a fullscreen TUI with backwards and forward buttons to let you edit previous answers.

Maybe expose an API that can be used by 3rd parties, and let someone else do that for you? LOL, so many ideas, so little time.

I'll do a PR in the next couple of days for the prompt change we've agreed. I'll replace the > with 2 spaces as per your earlier message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement good first issue Easy things for newbies help wanted The issue is valid, but we need community contributions to fix it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants