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

feat(hostname): add detect_env_vars as option #5196

Merged
merged 9 commits into from Sep 16, 2023

Conversation

mickimnet
Copy link
Member

@mickimnet mickimnet commented May 21, 2023

Hello everybody,

Motivation and Context

I don't use the default hostname module and instead use a custom module because I frequently use terminal multiplexers like tmux, screen, or Zellji. These multiplexers display the hostname in the status bar, so I don't want or need to see it in the terminal as well.

Description

To achieve this, I added the context::detect_env_vars function to the hostname module and also added a new context::has_no_negative_env_vars function to it.

This is the first time I've written anything other than shell scripts, so please bear with me🫣 I couldn't have done it without the invaluable help of my friend and experienced Rustacean, @dsander 💛

Option combinations

I created a truth table to outline the expected behavior of the changes:

Legend:

  • detect_env_vars:
    • positive: a name of an environment variable is configured and the variable is set
    • empty: the array is empty
  • detect_env_vars = none: none of the entered environment variables are set
  • detect_env_vars = negative: the name of an environment variable is prefixed with an "!" and the variable is set
ssh_only SSH_CONNECTION detect_env_vars result
true true positive show hostname
true true empty show hostname
true true none show hostname
true true negative show hostname
true false positive hide hostname
true false empty hide hostname
true false none hide hostname
true false negative hide hostname
false true positive show hostname
false true empty show hostname
false true none hide hostname
false true negative hide hostname
false false positive show hostname
false false empty show hostname
false false none hide hostname
false false negative hide hostname

Hint: If ssh_only is set to false and you still want to see the hostname in an ssh connection, you will need to add SSH_CONNECTION to the detect_env_vars array. My use case can be achieved with the following configuration:

[hostname]
ssh_only = false
detect_env_vars = [ '!TMUX', 'SSH_CONNECTION' ]

How Has This Been Tested?

  • I have tested using MacOS
  • I have tested using Linux
  • I have tested using Windows

Checklist:

  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

Thank you and have fun!
Mick

Edith says: exchanged the truth table with an actual table 😄

@mickimnet mickimnet changed the title Feat hostname detect env vars feat(hostname): add detect_env_vars as option May 21, 2023
src/context.rs Outdated
Comment on lines 249 to 251
env_vars.is_empty()
|| ((env_vars.iter().any(|e| self.get_env(e).is_some()))
&& self.has_no_negative_env_var(env_vars))
Copy link
Member

Choose a reason for hiding this comment

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

Instead of env_vars.is_empty() it might be better to use env_vars.any(|e| !e.starts_with('!')) to better support negative-only matchers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because of the supposed formula in #5196 (comment) we decided to leave it in.

Comment on lines 2194 to 2195
The `detect_env_vars` is only recognized, if `ssh_only` is set to `false`.
You can still check for a ssh connection, add `SSH_CONNECTION` to `detect_env_vars`.
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes more sense to join both with AND (or OR?) conditions.
If we decide to keep this behavior, it would warrant a log::warn! in the code.

Copy link
Member Author

@mickimnet mickimnet May 24, 2023

Choose a reason for hiding this comment

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

There is one combination, I'm unsure how to handle: what happens if an environment variable is configured for detect_env_vars and it is not set in the shell environment?

With ssh_only = "false", my logic would say: hide the hostname. With ssh_only = "true" and an active ssh connection, what should take precedence: the ssh_only or the detect_env_vars option?

See my comment for a new truth table.

@mickimnet
Copy link
Member Author

mickimnet commented May 24, 2023

As, I prefer to have it documented here, below is a new truth table for David's review comment:

Truth table for combining ssh_only with detect_env_vars

Legend:

  • detect_env_vars:
    • positive: a name of an environment variable is configured and the variable is set
    • empty: the array is empty
    • none: none of the entered environment variables are set
    • negative: the name of an environment variable is prefixed with an "!" and the variable is set
    • bold & ❗ shows changes to the truth table in the first post
ssh_only SSH_CONNECTION detect_env_vars result
true true empty show hostname (like now)
true true positive show hostname
true true none ❓ hostname
true true negative hide❗ hostname
true false empty hide hostname (like now)
true false positive show❗ hostname
true false none ❓ hostname
true false negative hide hostname
false true empty show hostname (like now)
false true positive show hostname
false true none hide hostname
false true negative hide hostname
false false empty show hostname (like now)
false false positive show hostname
false false none hide hostname
false false negative hide hostname

I'm unsure how to handle the two conditions, marked with ❓, in the above table.

@davidkna
Copy link
Member

davidkna commented May 28, 2023

I think it makes the most sense to consider both options mutually independent boolean variables.

$$\begin{align*} s &= \lnot \text{ssh\_only} \lor \text{SSH\_CONNECTION} \in \text{ENV} \\\ e &= \begin{cases} t & \text{if empty} \\\ f & \text{there is any negative match} \\\ t & \text{there are only negatives matchers} \\\ t & \text{there is a positive match} \\\ f & \text{otherwise} \end{cases} \end{align*}$$

In that case, I think it makes the most sense to enable the module if $s \land e$

@mickimnet
Copy link
Member Author

I believe the definition of $s$ and the combination of $s \land e$ is a good idea.

Regarding the cases of $e$, I'm not sure about the third one ($t \text{ there are only negatives matches}$). Why the different behavior between only and any negative matches?

@davidkna
Copy link
Member

davidkna commented Jun 4, 2023

The only part ("matchers") is concerned with the matchers themselves, so having only !-variables in the config.

The other one ("matches") is concerned with the evaluated matcher-matches.

mickimnet added a commit to mickimnet/starship that referenced this pull request Jul 2, 2023
The new `detect_env_vars` now requires either SSH_ONLY to be false or the
environment variable SSH_CONNECTION to be set, so that is will be used
@mickimnet mickimnet force-pushed the feat-hostname-detect_env_vars branch from 1375f68 to 2e58a5a Compare July 2, 2023 09:55
mickimnet added a commit to mickimnet/starship that referenced this pull request Jul 24, 2023
The new `detect_env_vars` now requires either SSH_ONLY to be false or the
environment variable SSH_CONNECTION to be set, so that is will be used
@mickimnet mickimnet force-pushed the feat-hostname-detect_env_vars branch from 8da0d9e to 53a149b Compare July 24, 2023 11:26
@mickimnet
Copy link
Member Author

Additionally, to the test on macOS, I did test the new feature on NixOS as well.

@mickimnet mickimnet requested a review from davidkna July 27, 2023 11:48
mickimnet added a commit to mickimnet/starship that referenced this pull request Jul 31, 2023
The new `detect_env_vars` now requires either SSH_ONLY to be false or the
environment variable SSH_CONNECTION to be set, so that is will be used
@mickimnet mickimnet force-pushed the feat-hostname-detect_env_vars branch from 90613d6 to 949661d Compare July 31, 2023 10:04
src/modules/hostname.rs Outdated Show resolved Hide resolved
src/context.rs Outdated Show resolved Hide resolved
src/context.rs Outdated Show resolved Hide resolved
mickimnet and others added 7 commits August 30, 2023 16:18
based on the newly added context::detect_env_vars

- extended context::detect_env_vars to check for negated environment
  variables as well, analogous to the other detect modules
- made hostname.detect_env_vars only active if ssh_only is set to false
  for backwards compatibility

Co-authored-by: Dominik Sander <dsander@users.noreply.github.com>
The new `detect_env_vars` now requires either SSH_ONLY to be false or the
environment variable SSH_CONNECTION to be set, so that is will be used
Co-authored-by: David Knaack <davidkna@users.noreply.github.com>
Co-authored-by: David Knaack <davidkna@users.noreply.github.com>
@mickimnet mickimnet force-pushed the feat-hostname-detect_env_vars branch from 08265a1 to 18a4225 Compare August 30, 2023 14:19
- fixed bracket error in hostname.rs, after changes
- updated comments for context.rs, for the suggested changes
docs/config/README.md Outdated Show resolved Hide resolved
Co-authored-by: David Knaack <davidkna@users.noreply.github.com>
@davidkna davidkna merged commit 43b2d42 into starship:master Sep 16, 2023
20 checks passed
@davidkna
Copy link
Member

Thanks for the contribution @mickimnet!

@mickimnet
Copy link
Member Author

Thank you so much, for your support @davidkna

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants