devbox icon indicating copy to clipboard operation
devbox copied to clipboard

boxcli,devbox: move default prompt into devbox.json

Open gcurtis opened this issue 3 years ago • 7 comments

Summary

We currently set the devbox shell prompt in the shellrc template with:

export PS1="(devbox) $PS1"

This breaks the prompt for users that have a theme or use a shell that sets the prompt in some other way. To allow user's to opt out of the prompt, move it into devbox.json where they can modify or delete it. Running devbox init will now generate a config that looks like:

{
  "packages": [],
  "shell": {
    "init_hook": [
      "# Here you can remove or customize the prompt for your project's devbox shell.",
      "# By convention, individual users can set DEVBOX_NO_PROMPT=1 in their shell's",
      "# rc file to opt out of prompt customization.",
      "if [ -z \"$DEVBOX_NO_PROMPT\" ]; then",
      "\tPS1=\"(devbox:tmp.wGAYMHRO) $PS1\"",
      "fi"
    ]
  }
}

This also means that users with an existing devbox config will no longer get a prompt. If they want to get the default prompt back, they can run:

devbox init -fix prompt

This fix will automatically append the default devbox prompt to the shell init hook unless there's already a command that contains PS1=. A future change will print a hint when launching a shell to tell the user about this change.

Note that the -fix flag accepts a comma-separated list of fixes in case we need to add more as the config evolves. For simplicity, the current implementation only allows "prompt", but can be redone later to support more.

How was it tested?

Manually running devbox shell after:

  1. Initializing a new config.
  2. Fixing an existing config.
  3. Fixing an already fixed config.

This one is trickier to add tests for because the prompt only applies for interactive shells, and the tests are obviously not interactive.

gcurtis avatar Sep 19 '22 19:09 gcurtis

Instead of checking that DEVBOX_NO_PROMPT is not set, shouldn't we check that DEVBOX_SHELL_ENABLED is set?

loreto avatar Sep 19 '22 20:09 loreto

The hook runs in the devbox shell, so it'll always be set.

Devbox itself doesn't do anything with the DEVBOX_NO_PROMPT environment variable. It serves more as an example of how a project can let individual users can opt out of the prompt, even thought a devbox project may want to set one. Users can rename the variable to something else or even ignore it entirely.

gcurtis avatar Sep 19 '22 20:09 gcurtis

I'm going to hold off on merging this until the next release. I want to add the shell hint that also informs the user of devbox init -fix and have them go out at the same time.

gcurtis avatar Sep 19 '22 20:09 gcurtis

I haven't reviewed the code, so this comment is purely based on the shell hook:

I was thinking we would move to a world where the prompt with (devbox) is opt-in instead of opt-out. In other words, we would delete the code where we modify the prompt, and instead we would add a default hook to devbox.json that modifies the prompt (if the user does not want to modify the prompt, they would just delete the hook).

Point taken that DEVBOX_SHELL_ENABLED is always set, so I guess the hook would just be to modify PS1 directly regardless of env variable.

loreto avatar Sep 19 '22 21:09 loreto

Yeah, that check for DEVBOX_NO_PROMPT is in devbox.json. It gets added to all new devbox.json files or if the user runs devbox init -fix prompt on an existing one.

We can simplify it down to just setting the prompt, but I thought it might be a good idea to include it by default. I'm imagining a scenario where a large project sets a prompt, but there are a few developers on the team where it messes up their shell.

gcurtis avatar Sep 19 '22 21:09 gcurtis

is this still for review? Or did y'all have a conversation out-of-band and plan more changes?

savil avatar Sep 22 '22 14:09 savil

@gcurtis what's the status on this one?

dropping myself to clear my review queue, but feel free to add me back.

savil avatar Oct 20 '22 16:10 savil