Skip to content

Conversation

gcurtis
Copy link
Collaborator

@gcurtis gcurtis commented Sep 19, 2022

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.

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.
@loreto
Copy link
Contributor

loreto commented Sep 19, 2022

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

@gcurtis
Copy link
Collaborator Author

gcurtis commented Sep 19, 2022

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
Copy link
Collaborator Author

gcurtis commented Sep 19, 2022

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.

@loreto
Copy link
Contributor

loreto commented Sep 19, 2022

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.

@gcurtis
Copy link
Collaborator Author

gcurtis commented Sep 19, 2022

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.

@savil
Copy link
Collaborator

savil commented Sep 22, 2022

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

Copy link
Contributor

@loreto loreto left a comment

Choose a reason for hiding this comment

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

LGTM

@savil savil removed their request for review October 20, 2022 16:48
@savil
Copy link
Collaborator

savil commented Oct 20, 2022

@gcurtis what's the status on this one?

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

@mikeland73 mikeland73 removed their request for review October 21, 2022 17:50
@gcurtis gcurtis closed this Jan 22, 2023
@gcurtis gcurtis deleted the gcurtis/prompt branch January 22, 2023 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants