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

Export multiline prompt functions #7675

Merged
merged 1 commit into from
Nov 1, 2023
Merged

Conversation

josevalim
Copy link
Contributor

@josevalim josevalim commented Sep 23, 2023

Both shell:default_multiline_prompt/1 and
shell:inverted_space_prompt/1 have been exported.

shell:prompt_width/1 is now also available as a
helper function for custom prompt implementations.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 23, 2023

CT Test Results

       3 files     145 suites   1h 37m 59s ⏱️
3 373 tests 3 072 ✔️ 301 💤 0
3 892 runs  3 538 ✔️ 354 💤 0

Results for commit 8fc5c40.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@josevalim
Copy link
Contributor Author

On a second though, it may be better to expose the underlying prim_tty:npwcwidthstring/1 function, so it is easier for others to build their own prompts, including the default one. However, prim_tty is not public. Perhaps we can expose it on edlin, shell, or some other module?

@IngelaAndin IngelaAndin added the team:VM Assigned to OTP team VM label Sep 23, 2023
@dgud
Copy link
Contributor

dgud commented Sep 23, 2023

You can set the prompt with the documented:
shell:prompt_func(PromptFunc)
but there needs to be a
shell:multiline_prompt_func(MPF)

@frazze-jobb frazze-jobb self-assigned this Sep 25, 2023
@frazze-jobb
Copy link
Contributor

I agree, the prim_tty:npwcwidthstring/1 will need to be made available through some public module I suggest shell.
And we need a call as @dgud mentioned to set the multiline prompt via the shell module.
Since the shell_prompt and multiline_prompt is related, it would probably make more sense to have both the default_mulitline_prompt and inverted_space_prompt available through the shell module. And documentation of them in the shell module.

@josevalim
Copy link
Contributor Author

I will send a PR this week with most of these changes. :)

@josevalim josevalim changed the title Export edlin:default_multiline_prompt/1 Export multiline prompt functions Sep 28, 2023
@josevalim
Copy link
Contributor Author

I have rebased and moved the multiline prompt functions to shell, with tests and docs. I did not add a convenience API for setting the shell multiline prompt programatically.

I have decided to rename prim_tty:npwcwidthstring/1 to shell:prompt_width/1, as the first name was a good match for prim_tty (and its internal functions) but it felt out of replace in the shell module. I will be glad to rename it to something else.

@frazze-jobb
Copy link
Contributor

frazze-jobb commented Oct 28, 2023

Thanks @josevalim, I think it looks good, just the failing testcase that needs fixing.
I can try to find time to create the convenience API shell:multiline_prompt_func(MPF) later in a separate PR.

Both shell:default_multiline_prompt/1 and
shell:inverted_space_prompt/1 have been exported.

shell:prompt_width/1 is now also available as a
helper function for custom prompt implementations.
@josevalim
Copy link
Contributor Author

I changed the test but I realized now that edlin:inverted_space_prompt was somewhat public then, even if not documented. Should we keep it around for compatibility or will we tell people to update their flags accordingly, given this is still new (experimental?)?

@frazze-jobb
Copy link
Contributor

Still experimental so. And it makes more sense to me to have it in shell

@frazze-jobb frazze-jobb added the testing currently being tested, tag is used by OTP internal CI label Oct 30, 2023
@frazze-jobb frazze-jobb merged commit 11b05b8 into erlang:master Nov 1, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants