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

Module configuration with format strings #624

Closed
matchai opened this issue Nov 4, 2019 · 35 comments
Closed

Module configuration with format strings #624

matchai opened this issue Nov 4, 2019 · 35 comments
Assignees
Labels
💬 discussion Conversation to figure out actionable steps. Most feature ideas start here. ✨ enhancement A new feature implementation.
Milestone

Comments

@matchai
Copy link
Member

matchai commented Nov 4, 2019

With more and more configuration being introduced into Starship, we're starting to see a fair bit of configuration bloat that simply modifies the formatting of the output, rather than changing the way the prompt retrieves information or processes context. (e.g. #603, #623)

What I would like to propose is the following configuration:

[cmd_duration]
# ${duration} - The duration of the last running command

format = "took ${duration}!"

This would allow us to remove all the following configuration options in modules where they are static:

  • symbol
  • prefix
  • suffix

In modules where the values are dynamic, they would remain. For example, the character module:

[character]
# ${prompt_symbol} - The prompt symbol

format = "${prompt_symbol} "
symbol = ""
error_symbol = ""

We could also remove the following configuration options from git_status, where enabling them would be implicit by them being in the format string:

  • conflicted_count
  • untracked_count
  • modified_count
  • etc...

This would, of course, require some changes in the way we educate users, but I think it would make for a less dense and complicated configuration system in the long run.

@matchai matchai added ✨ enhancement A new feature implementation. 💬 discussion Conversation to figure out actionable steps. Most feature ideas start here. labels Nov 4, 2019
@williamluke4
Copy link

I think this is much better than the current method

@jletey
Copy link
Member

jletey commented Nov 4, 2019

@matchai I think that this is a splendid idea, and that it simplifies the configuration file significantly (and possibly the code as well 🤞).

As you said, this removes the hassle of configurable prefixes/suffixes of modules, which are inconsistent for each module at the moment.

IMO, I think that this will be a good thing to implement before we go stable, and I'm more than willing to help implement this feature.

Also, this is related in part to #346.

@heyrict
Copy link
Member

heyrict commented Nov 6, 2019

@matchai What about the styles?

One option that I initially came about is to use something similar to the escaping sequence, e.g. format = "${style=fg:green bg:blue}${prompt_symbol}${style=none}".

But I think adding parameters like format = "${prompt_symbol?style=fg:green bg:blue}" makes more sense.

@williamluke4
Copy link

@heyrict I really like the second one
format = "${prompt_symbol?style=fg:green bg:blue}"

@heyrict
Copy link
Member

heyrict commented Nov 8, 2019

What about using an array? It will be easier to parse (and we can pass arguments other than string to modules). Also, using this syntax allows us to add the same module multiple times with different config, which is useful if we want to display multiple environment variables.

[[modules]]
name = "character"
config = {
  symbol = "",
  error_symbol = "",
}
segments = [
  { name = "prompt_symbol", style = "bold red" },
  { name = "text", value = " " },
]

I think json format may look better if we have highly nested tables.

{
  "modules": [
    {
      "name": "character",
      "config": {
        "symbol": "❯",
        "error_symbol": "✖"
      },
      "segments": [
         // Add a `>` symbol if $HOSTNAME == home, related to #607
        {
          "name": "match",
          "matches": [
            {
              "env": "HOSTNAME",
              "eq": "home",
              "display": {
                "name": "text",
                "value": ">",
                "style": "bold green"
              }
            }
          ]
        },
        // Add segment `prompt_symbol`
        { "name": "prompt_symbol", "style": "bold red" },
        {
          "name": "text",
          "value": " "
        }
      ]
    }
  ]
}

@jletey
Copy link
Member

jletey commented Nov 8, 2019

@heyrict Although I think that that is an ingenious solution, it doesn't seem to make the configuration file any simpler, which was one of the goals of this discussion.

@heyrict
Copy link
Member

heyrict commented Nov 8, 2019

I've skimmed through crates.io and didn't find a crate that meets the need of #624 (comment).

So I made a runtime string formatter at https://github.com/heyrict/spongy as a PoC.

Feel free to ping me if you found some existing crates that can do the trick.

@heyrict
Copy link
Member

heyrict commented Nov 29, 2019

Quick update: I've rewritten prompt_order config with format strings in feat/format-string. It works but I'm a little concerned about the performance (about ~10 ms delay than master).

Note: I'll be busy before Jan 10. Feel free to continue working on this branch if you would like to.

@matchai
Copy link
Member Author

matchai commented Dec 12, 2019

@heyrict Sorry I didn't get around to answering #624 (comment), which I think is a discussion we need to have before #682 can go through.

I personally find the ${prompt_symbol?style=fg:green bg:blue} syntax to be a little too programatic and difficult to read for something we are embedding in TOML configuration.

Giving it a little more thought, I think something along these lines might be a little easier to make sense of:

# Previous:
"via ${styled?value=⬢ &style=green bold}${version?style=green bold} "

# Alternative:
"via [⬢ $version](green bold) "

The above is inspired by Markdown syntax. I'm not attached to [](), but I think that something more in that direction will make configuration simpler.

Anything prefixed with an unescaped $ would be handled as a variable, and would otherwise be a string.

What are your thoughts?

@heyrict
Copy link
Member

heyrict commented Dec 12, 2019

@matchai I'm not opposed to [](), but when I am trying to get a working example of powerline prompt, I found the need to passing extra parameters to a segment based on the user's need.

We may want:

directory
|   git_branch 
↓   ↓
~ > master >
↑ ↑ ↑      ↑
| | |      fg:cyan
| | bg:cyan fg:white
| *bg:cyan fg:red*
bg:red fg:white

if we are in a condition that git_branch renders, but when git_branch does not render, we may want:

directory
↓
~ >
↑ ↑
| *fg:red*
bg:red fg:white

This cannot be done with merely []() syntax. We have to make a compromise between simplicity and flexibility.

As for me, anything that can be parsed as a Map looks good, but []() which is parsed as an (a, b) tuple, may not cover all user's needs.

See Also: Limitation section in #682

@lucarin91
Copy link
Contributor

Hi, I was thinking about a possible solution that mixes together both the solution of @heyrict and @matchai.

The idea is to divide the formatting configuration of the prompt from the logical configuration. Thus, create a prompt_style parameter that represents the style format of the entire prompt. The configuration is an array of string with the @matchai format that will be eventually joined together.
Here the prompt_style array for the default configuration:

prompt_style = [
   "[$username.text]($username.style)",
   "[$hostname.text](bold dimmed green)",
   "[☸ $kubernetes](bold blue)",
   "[$directory.text](bold cyan)",
   "[ $git_branch.text](bold purple)",
   "[$git_state.text](bold yellow)",
   "[\[$git_status.symbol[$git_status.text](dimmed red)\]](bold red)",
   "[ $hg_branch.text](bold purple)",
   "is [📦 $package.text](bold red)",
   "via [•NET $dotnet.text](bold blue)",
   "via [🐹 $golang.text](bold cyan)",
   "via [☕ $java.text](dimmed red)",
   "via [⬢ $nodejs.text](bold green)",
   "via [🐍 $python.text](bold yellow)",
   "via [💎 $ruby.text](bold red)",
   "via [🦀 $rust.text](bold red)",
   "via [$nix_shell.text](bold red)",
   "via [C $conda.text](bold green)",
   "[🐏 $memory_usage.text](bold dimmed white)",
   "[☁️ $aws.text](bold blue)",
   "with [$env_var.text](dimmed black)",
   "[took $cmd_duration.text](bold yellow)",
   "\n",
   "[✦$jobs.text](bold blue)",
   "[$battery.symbol$battery.text](bold red)",
   "[$time.text](bold yellow)",
   "[$character.symbol](bold green)",
]

As shown in the example, each module can have a .text .style and .symbol variable, which represents the dynamic parts of the module. For instance $rust.text will be substituted with the version of Rust, instead $character.symbol will be substituted with the symbol for success or error.
In the configuration is possible to set the different possibility for this dynamic variable, as shown in the following:

[[username.style]]
root = "bold red"
user = "bold yellow"

[[git_state.text]]
rebase = "REBASING"
merge = "MERGING"
...

Sorry for the very long comment, I hope that I was clear and that the proposal was pertinent.

@lzybkr
Copy link

lzybkr commented Dec 12, 2019

@heyrict I have PowerLine working well enough for my needs, see #528 (comment) and this commit https://github.com/lzybkr/starship/commit/3e02f172adcf5cf84022fd14b10c52285a4bde6e

I've avoided a PR because in my mind, the proposed changes here would simplify PowerLine styling even more.

@matchai
Copy link
Member Author

matchai commented Dec 12, 2019

Unfortunately I don't think it's worth this much complexity for the Powerline use-case. We could get around to adding advanced configuration for this in particular, but >90% of users will only be working with basic configuration.

There is certainly room is extend the syntax to support advanced configuration, but I think that's a discussion for another time.

@heyrict
Copy link
Member

heyrict commented Dec 12, 2019

What I mean by powerline syntax is that, users may want a full control over the command prompt. I agree that most user will be working with the basic configuration, but requests do exist for more customization.

Other issues that I personally would like to solve by a Map is to have multiple modules (esp. env_var) with different parameters, see #664

There is certainly room is extend the syntax to support advanced configuration, but I think that's a discussion for another time.

Well, I would rather get it discussed before we've gone too far. There are feature request of all kind. I would like to know what we want to cover and what not, as any refactor is costly and it would be painful to have two if we can get things done with one.

@matchai
Copy link
Member Author

matchai commented Dec 12, 2019

I'm hesitant to support maps when something similar can perhaps be achieved with TOML configuration.
For instance, perhaps env_var would allow for custom keys to distinguish between environment variables:

[env_var]
format = "[$session](red) [$shell](blue)"

[env_var.session]
variable = "SESSION"

[env_var.shell]
variable = "SHELL"

I think there would be a more elegant way to use maps in TOML than to have another map added within configuration. At the end of the day, I'd rather have simple configuration than more extensibility.

@matchai
Copy link
Member Author

matchai commented Jan 5, 2020

@chipbuster I figured you might have some thoughts to share, if you haven't had the chance to see this issue yet. 😊

@chipbuster
Copy link
Contributor

@matchai Funnily enough, I had just been reading over this and another issue/PR when you tagged me (then I had to travel and clean up a bunch of stuff, so I'm a little delayed in responding).

@heyrict I looked over your benchmark on #682. Was that 30% increase just for two modules with custom format strings? If so, the cost of the current format may be intolerably high. On a slow computer, the default prompt can take 50-100ms to render without too many external calls--and if the cost of custom format strings increases per-string, then using too many format strings might push prompt render time well above 200ms, which is too slow.

@heyrict
Copy link
Member

heyrict commented Jan 9, 2020

@chipbuster Thanks for your review. I haven't figured out which part of the code is responsible for the increment to the render time, but it seems that no matter how many modules there are, the render time is always increased by ~30% on my laptop.

@chipbuster
Copy link
Contributor

@heyrict Thanks, that solve some concerns, but makes this more mysterious. I'll see if I have some time this weekend to rip out the old perf stack and see where the increase is coming from. If it's some static overhead then I guess there's not much to be done, but if our parsing is somehow slowing things down, there's definitely something to be said for making a simpler configuration language which can be parsed faster (or maybe even handrolling an FSM).

I'd want to be careful about going too complex on the scripting language here: I think one of our biggest advantages over other shell prompts is ease of understanding the configuration, and it would be problematic if we lost both that and our relative speed (compared to say, the default omzsh prompts) in order to gain a configurability advantage. I don't think we'll ever be able to match actual shell-based prompts for configurability, simply because we're an external binary interfacing with the shell at very fixed points while shell-based prompts write in the actual language of the shell.

Of course, if we can get the ability to do powerline without sacrificing too much in terms of simplicity/speed, we should go for it. I guess we'll see what the profiler turns up.

@chipbuster
Copy link
Contributor

@heyrict It took me a while to poke around here, because the code structure is drastically different between the format-string branch and its common ancestor with the main branch (which merge-base says is f458a5e).

One thing that jumped out at me was that a huge amount of time (71%) in the base version of the code is spent in a child of rayon_core::registry::ThreadBuilder::run, while that function is entirely missing in the trace for your code.

My best guess here is that you accidentally stripped out the parallelism in print.rs while making your updates, so you're comparing a parallel run (in the base code) against a serial one (in your branch), which comes up 30% slower.

If I replace the call to par_iter() in the base code with a call to iter(), (file src/print.rs line 43, commit f458a5e) and time that, I get that with a blank config, your code take 23.6 ms and the base code takes 23.4ms: not a significant difference.

Could you try either disabling parallelism on the base commit or re-enabling it on your branch and re-running your benchmarks?

@06kellyjac
Copy link

I'd like this feature a bunch because it'd give me more freedom.

I've spent more time than I'd like to admit playing with prefix & suffix and how not every module has the first, the second, or sometimes misses both.

On my old home made version of spaceship my path prompt looked like so: [~/something] with the path being a different colour to the brackets. With string formatting I could easily and prefixes and suffixes to whatever I like & set colours on various different parts.


Not really related but in the same vein of freedom: I'd also like the ability to provide arbitrary functions or something to run. With my other prompt my path was similar to the fish style one provided by starship now but it's length was variable depending on the uniqueness of the path rather than a strict length.

@esposm03
Copy link
Contributor

I think the styles would be much better if specified with an xml-like syntax (e.g. <red><bold>Hello</bold></red>). That would be more readable and easier to implement, in my opinion. In the future, it would also allow for conditional styles by substitution (something like #570).

@heyrict
Copy link
Member

heyrict commented Mar 20, 2020

I'm going to ping @chipbuster and @matchai here if you have any thoughts after two months :)

I'm pretty fine with #624 (comment) if you think there is no need to adds more functions to format strings.

@matchai
Copy link
Member Author

matchai commented Mar 20, 2020

I'm pretty fine with #624 (comment) if you think there is no need to adds more functions to format strings.

I think that would be best for the time being. 👍

I'd be open to eventually adding an additional "advanced" configuration style once we finish with the "simple" configuration implementation. That's certainly worth opening a separate issue to track and discuss.

@johnletey
Copy link
Contributor

@matchai With the merge of #1021, what is the plan for moving forward? I'm more than happy to start converting the modules to the new format :)

@mickimnet
Copy link
Member

In reply to @matchai comment on advanced configuration:

Unfortunately I don't think it's worth this much complexity for the Powerline use-case. We could get around to adding advanced configuration for this in particular, but >90% of users will only be working with basic configuration.

You're right in regards to the majority of users not having much interest in configuration of tools by themself, but – IMHO – they would use a theme to get a specific visual representation they prefer, if said theme(s) are available.

In regards to enabling a Powerline style of the Starship prompt, I believe there is more to todo than just an advanced configuration. In order to have prompts like this (I included the default and current theming options for reference):
Screenshot 2020-04-13 at 14 34 09
You would need to introduce a separator in between the modules. In order for the separator to look like in the above example, the foreground color of the separator needs to be the background color of the previous module and the background needs to be the same as the background of the following module.

Therefore either the modules have to be self-aware which module was active before them or you need to check the active modules and insert the separator in between.

If I was just stating the obvious, please excuse my intrusion.

I personally would really like the possibility to use a Powerline theme for Starship, as I consider it the best prompt by far (information and configuration wise, just not visually, ATM 😉)!

@andymac4182
Copy link
Contributor

I have just attempted to implement a 2nd variable for the dotnet module to support showing the TFM the current project is building against. The problem I ran into was with format strings there isn't an easy way to define a prefix for the 2nd variable that has its visibility linked to that variable. I thought that was how the TextGroup would would work but realized that it was just for text formatting.

Is there a preferred way to handle this? Does it need a separate variable for that prefix?

@heyrict
Copy link
Member

heyrict commented Apr 16, 2020

@andymac4182 IMO, you can either use an extra format string like this and then make it renders conditionally programmatically.

[some_module]
format = "via $version$branch"
branch_format = "@$branch"

Or allow users to define their own variables.

[some_module]
format = "via $version$custom"

[some_module.variables]
custom = "@$branch"

, and then display the custom variables only when all variables are not None.
This is currently possible (and does not require much effort) but not documented or having an example.

We may need some best practices to follow.
@matchai here if you have some ideas.

@stku1985
Copy link
Contributor

Since the format parser landed in 0.40 I wanted to port the kubernetes module to use format strings. But I did run into the following issue:
The kubernetes prompt looks like this: [☸ $context \\($namespace\\)](bold cyan). But the parentheses should only be displayed if $namespace is not "".

Hence my questions:
Can this already be done in the current formatter?
If not, what should be done to port such modules? I see two options:

  • mutate the format string in the module (ugly, partially defeats the purpose of having a uniform format syntax)
  • extend the formatter to handle this case.

I am looking forwards to reading your opinions / answers.

@andytom
Copy link
Member

andytom commented Apr 24, 2020

Since the format parser landed in 0.40 I wanted to port the kubernetes module to use format strings. But I did run into the following issue:
The kubernetes prompt looks like this: [☸ $context \\($namespace\\)](bold cyan). But the parentheses should only be displayed if $namespace is not "".

Hence my questions:
Can this already be done in the current formatter?
If not, what should be done to port such modules? I see two options:

  • mutate the format string in the module (ugly, partially defeats the purpose of having a uniform format syntax)
  • extend the formatter to handle this case.

I am looking forwards to reading your opinions / answers.

@stku1985, that sounds like the same issue I raised in #1116 (comment).
Some possible work answers have been posted there but it would be good to get your input too.

@matchai matchai unpinned this issue May 7, 2020
@andytom andytom added this to the v0.45.0 milestone Sep 8, 2020
@andytom
Copy link
Member

andytom commented Sep 19, 2020

Since we are getting close to the v0.45.0 release, I'm going to close this issue so we can see what is actually left to do in the milestone and what still needs a little bit of work.

@andytom andytom closed this as completed Sep 19, 2020
dagbrown pushed a commit to dagbrown/starship that referenced this issue Oct 22, 2021
This PR implements the parser of format strings described in starship#624.
@vladimir-lu
Copy link
Contributor

@mickimnet
I've just added #4945 to allow referencing the previous foreground/background color (in the format string or module level) which I think solves that issue

@mickimnet
Copy link
Member

mickimnet commented Mar 6, 2023

@vladimir-lu, thank you, I saw your PR, and I am looking forward to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💬 discussion Conversation to figure out actionable steps. Most feature ideas start here. ✨ enhancement A new feature implementation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.