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

make the terminal directive more consistent #34

Merged
merged 3 commits into from
Apr 15, 2024
Merged

Conversation

akcano
Copy link
Collaborator

@akcano akcano commented Feb 28, 2024

This is a conceptual PR to suggest a breaking change in the behaviour of the .. terminal directive. Currently, there are two issues:

  1. The :input: serves two purposes: it's an option (I suppose this was done to provide a shorthand one-line version of the directive) and a prefix to disambiguate commands from output, which may be confusing for people who are familiar with reSrtucturedText but not this extension.
  2. The non-root prompt includes a hardcoded home directory designation (~), which kinda defies the purpose if the command is supposed to occur elsewhere.

The contra part here is obvious: this breaks the directive as it's currently used. Let's discuss.

@ru-fu
Copy link
Contributor

ru-fu commented Mar 12, 2024

I'm fine with the prompt change.

However, I don't understand the problem with the :input: field. In the basic case (which is probably 90% of all my uses of the terminal directive), I'll have one input line, and the rest is output. So :input: is the field that provides the terminal input, and this is much clearer imo than parsing the content of the directive.
I understand that it can be slightly confusing when you have several input lines, but then I think it's still easier to use the "same" syntax for all input lines.

This is definitely not a serious UX issue that warrants breaking the extension.

What I would suggest is to make the :input: field fully optional - if you don't specify it, the directive shouldn't show the prompt.
(Currently, you can already leave it out, but you'll get an empty prompt then.)
If we change this, the directive can be used in one of two ways: either with a single input line (specified as an option field) or with several input lines (specified as parts of the directive content). This will leave the existing functionality in place, so nothing will break, but we can update the readme to promote using one of the two ways and not mix them.

@akcano
Copy link
Collaborator Author

akcano commented Mar 13, 2024

The only problem arises when :input: occurs mid-content; it looks confusing. The syntax clearly suggests it's a directive option, and directive options usually follow their directives immediately. It's bearable, we can live with this internally, but distributing non-idiomatic directives is something we can avoid.

I'm OK with your suggestion, though, because the confusion mainly arose when seeing these two prompt types used together. In fact, I'd go further and make these two styles mutually exclusive; what do you think?

@ru-fu
Copy link
Contributor

ru-fu commented Mar 13, 2024

The syntax clearly suggests it's a directive option,

I chose the syntax mainly so you wouldn't need to remember two different versions - but also to make sure we don't run into problems where you would need exactly that format in the terminal output. I think :input: is quite unlikely to appear at the beginning of a line in a terminal, while $$$ is probably also not common, but I could imagine it being used as a divider or something in script output.

In fact, I'd go further and make these two styles mutually exclusive; what do you think?

I would document them to be mutually exclusive, but not actually enforce it. Enforcing it would be a breaking change, and I don't see a need for that.

Copy link
Contributor

@ru-fu ru-fu left a comment

Choose a reason for hiding this comment

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

This is failing now (see the action).

Can you also update the readme?

Copy link
Contributor

@ru-fu ru-fu left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

Can you increase the version number in setup.cfg (otherwise I won't be able to publish 😉 )?

@akcano
Copy link
Collaborator Author

akcano commented Apr 15, 2024

Looks good, thanks!

Can you increase the version number in setup.cfg (otherwise I won't be able to publish 😉 )?

done!

Copy link
Contributor

@ru-fu ru-fu left a comment

Choose a reason for hiding this comment

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

Thanks!

@ru-fu ru-fu merged commit d03d0cd into canonical:main Apr 15, 2024
1 check passed
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.

2 participants