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

Command Line: Add support for command termination chars #3344

Merged
merged 7 commits into from
Feb 23, 2022

Conversation

at055612
Copy link
Contributor

@at055612 at055612 commented Feb 18, 2022

Adds attribute line-termination-str to support multi-line commands in CLIs that use command termination chars instead of line continuation chars, e.g. SQL.

Follows on from the merged PR #3326

image

Adds attribute line-termination-str
@github-actions
Copy link

github-actions bot commented Feb 18, 2022

JS File Size Changes (gzipped)

A total of 1 files have changed, with a combined diff of +24 B (+2.1%).

file master pull size diff % diff
plugins/command-line/prism-command-line.min.js 1.14 KB 1.16 KB +24 B +2.1%

Generated by 🚫 dangerJS against 1c8408c

@RunDevelopment
Copy link
Member

I'm honestly not a fan of this for multiple reasons.

Firstly, it's brittle. The termination string completely disregards the actual syntax of the command language. This might be okay in the MySQL use-case you presented, but many other languages with a shell-like repl count brackets instead of relying on a statement terminator (in this case, a ;). I can already see people requesting that this feature should support languages like scheme, where we would have to count parenthesis, which requires actually parsing the language.

Side note: While the argument of disregarding syntax also somewhat applied to #3326, most shell-like languages are compatible with a simple data-continuation-str suffix. Non-shell-like languages typically are statement or expression oriented, so the data-continuation-str suffix is irrelevant for them. As such, #3326 provide a nice feature geared toward shell-like languages.

Secondly, it changes the way line are categorized fundamentally. Prior to #3326, each line was their own; a line was either output or a command. #3326 changed that by adding the ability to merge adjacent command lines into one command. This change was conceptually built on top of how Command line categorized lines before.

However, this PR work fundamentally different. With a termination string, we have to assume that all command lines are one block and then we split up that block using the terminator. So now we have 2 principals that work against each other: We have a bottom-up approach that tries to merge adjacent commands and a top-down approach that tries to split commands.
You tried to marry these two approaches, which resulted in the very complex if conditions that are at the heart of this PR.


This was my wordy explanation as to why I think that line-termination-str is the wrong approach.

A simpler approach might be to add something like a contiuation-prefix attribute. All command line starting with this prefix will be assumed to be continuations (this is similar to data-filter-output). This would nicely fit together with #3326 as #3326 would conceptually just add that prefix for. (This is not how #3326 is implemented, but users can think of it working like that.)

@at055612
Copy link
Contributor Author

@RunDevelopment thanks for looking at the PR. I hadn't appreciated that there are repls that parse the braces/brackets to determine continuation (is seems Scala is another example of this). In light of this I agree the approach of this PR is flawed.

The if logic did get a bit complex, partly because I was trying to let it support both continuation and termination at the same time (no doubt somebody would want that at some point) rather than it being one mode or the other with one attribute trumping the other.

Are you proposing removing data-continuation-str and just having continuation-prefix (or maybe call it data-filter-continuation to be consistent with the output filter)? i.e. for bash

<pre class="command-line" data-filter-output="(out)" data-filter-continuation="(con)" >
<code class="language-bash">export MY_VAR=123
echo "hello"
(out)hello
echo one \
(con)two \
(con)three
(out)one two three
(out)
echo "goodbye"
(out)goodbye</code>
</pre>

...OR having both data-filter-continuation and data-continuation-str?

I can see how using a continuation prefix gives total control to the content author and means Prism doesn't have to cater for all the different ways languages handle continuation.

@RunDevelopment
Copy link
Member

having both data-filter-continuation and data-continuation-str?

Yes, that one. I think data-continuation-str is a feature worth keeping. Most people are probably going to use the plugin for shell-like languages, so data-continuation-str will help a lot of users.

@at055612
Copy link
Contributor Author

Having both attrs works for me.

Are you happy with data-filter-continuation as a name?

@RunDevelopment
Copy link
Member

Are you happy with data-filter-continuation as a name?

Yes, sounds good.

@at055612
Copy link
Contributor Author

@RunDevelopment Please see the revised code and docs.

Copy link
Member

@RunDevelopment RunDevelopment 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! I really like the changes you made to the docs.

Just 2 minor things:

plugins/command-line/prism-command-line.js Outdated Show resolved Hide resolved
plugins/command-line/index.html Show resolved Hide resolved
@RunDevelopment RunDevelopment merged commit b53832c into PrismJS:master Feb 23, 2022
@RunDevelopment
Copy link
Member

Thank you for contributing @at055612!

@at055612 at055612 deleted the command-line-termination branch February 23, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants