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

implement a prompt_hostname function #3482

Merged
merged 3 commits into from Oct 23, 2016

Conversation

Projects
None yet
3 participants
@krader1961
Contributor

krader1961 commented Oct 23, 2016

Quick sanity check please that I didn't introduce a typo.

Standardize how the host name is included in the prompts that do so.

Fixes #3480

implement a fish_prompt_hostname function
Standardize how the host name is included in the prompts that do so.

Fixes #3480
@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Oct 23, 2016

Member

Did you forget to commit the function?

Member

faho commented Oct 23, 2016

Did you forget to commit the function?

@floam

fish: Unknown command 'fish_prompt_hostname'

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Oct 23, 2016

Member

Should this maybe be prompt_hostname? Simply for discoverability, and to match prompt_pwd?

Member

floam commented Oct 23, 2016

Should this maybe be prompt_hostname? Simply for discoverability, and to match prompt_pwd?

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Oct 23, 2016

Contributor

Did you forget to commit the function?

Oops, yes, stupid git commit -a doesn't include new files. Fixed.

Contributor

krader1961 commented Oct 23, 2016

Did you forget to commit the function?

Oops, yes, stupid git commit -a doesn't include new files. Fixed.

@floam

floam approved these changes Oct 23, 2016

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Oct 23, 2016

Member

Is splitting on '.' from hostname output always equivalent to using hostname -s?

(I genuinely have no idea.)

Member

floam commented Oct 23, 2016

Is splitting on '.' from hostname output always equivalent to using hostname -s?

(I genuinely have no idea.)

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Oct 23, 2016

Contributor

Should this maybe be prompt_hostname? Simply for discoverability, and to match prompt_pwd?

Agreed. Changed.

Contributor

krader1961 commented Oct 23, 2016

Should this maybe be prompt_hostname? Simply for discoverability, and to match prompt_pwd?

Agreed. Changed.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Oct 23, 2016

Contributor

Is splitting on '.' from hostname output always equivalent to using hostname -s?

It's supposed to be since the definition of hostname -s is to convert a FQDN to a simple host name.

Contributor

krader1961 commented Oct 23, 2016

Is splitting on '.' from hostname output always equivalent to using hostname -s?

It's supposed to be since the definition of hostname -s is to convert a FQDN to a simple host name.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Oct 23, 2016

Member

In no case can the FQDN be something like "www.mysite.com" and hostname -s and the simple host name be "myserver"? That'd be my only worry if it's possible, it'd now output "www".

Member

floam commented Oct 23, 2016

In no case can the FQDN be something like "www.mysite.com" and hostname -s and the simple host name be "myserver"? That'd be my only worry if it's possible, it'd now output "www".

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Oct 23, 2016

Contributor

If by simple host name you mean what is returned as the nodename by the uname(3) then "yes" in theory. In practice probably not because a) that value is supposed to be the network name and should match whatever gethostname() returns. If it doesn't the system is misconfigured and that isn't our problem.

Contributor

krader1961 commented Oct 23, 2016

If by simple host name you mean what is returned as the nodename by the uname(3) then "yes" in theory. In practice probably not because a) that value is supposed to be the network name and should match whatever gethostname() returns. If it doesn't the system is misconfigured and that isn't our problem.

@floam floam changed the title from implement a fish_prompt_hostname function to implement a prompt_hostname function Oct 23, 2016

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Oct 23, 2016

Member

Is splitting on ',' from hostname output always equivalent to using hostname -s?

First of all, that's what we currently do, so there's no regression here. Secondly, this allows us to fix it in one place if we ever figure out a better solution.

Thirdly: Probably not. This is probably one of those things where there's a super-rare super-complicated edge case that some people get really passionate about (maybe some way to make it recursive, or dependent on locale?). We'll get an issue and then we can work it out. Or they can just override the function.

Member

faho commented Oct 23, 2016

Is splitting on ',' from hostname output always equivalent to using hostname -s?

First of all, that's what we currently do, so there's no regression here. Secondly, this allows us to fix it in one place if we ever figure out a better solution.

Thirdly: Probably not. This is probably one of those things where there's a super-rare super-complicated edge case that some people get really passionate about (maybe some way to make it recursive, or dependent on locale?). We'll get an issue and then we can work it out. Or they can just override the function.

@faho faho added this to the next-2.x milestone Oct 23, 2016

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Oct 23, 2016

Member

First of all, that's what we currently do, so there's no regression here

We used to use hostname -s here:

   # Just calculate these once, to save a few cycles when displaying the prompt
    if not set -q __fish_prompt_hostname
        set -g __fish_prompt_hostname (hostname -s)
    end
Member

floam commented Oct 23, 2016

First of all, that's what we currently do, so there's no regression here

We used to use hostname -s here:

   # Just calculate these once, to save a few cycles when displaying the prompt
    if not set -q __fish_prompt_hostname
        set -g __fish_prompt_hostname (hostname -s)
    end
@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Oct 23, 2016

Member

Yeah, but in the other prompts we didn't. This was inconsistent.

Member

faho commented Oct 23, 2016

Yeah, but in the other prompts we didn't. This was inconsistent.

@krader1961 krader1961 merged commit 37d91d0 into fish-shell:master Oct 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@krader1961 krader1961 deleted the krader1961:fish-prompt-hostname branch Oct 23, 2016

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Oct 24, 2016

Member

I happen to notice here (not related to this particular commit or it's goal) that it might be beneficial to have this as @(prompt_hostname) -- then anyone can override prompt_hostname with a function that does nothing to leave the hostname and the at-symbol out of their prompt entirely without editing the prompt due to how the cartesian product works.

Member

floam commented on share/functions/fish_fallback_prompt.fish in 07de13f Oct 24, 2016

I happen to notice here (not related to this particular commit or it's goal) that it might be beneficial to have this as @(prompt_hostname) -- then anyone can override prompt_hostname with a function that does nothing to leave the hostname and the at-symbol out of their prompt entirely without editing the prompt due to how the cartesian product works.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Nov 1, 2016

Member

It looks like most of the sample prompts are not using this.

Member

floam commented Nov 1, 2016

It looks like most of the sample prompts are not using this.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Nov 1, 2016

Member

Er, I'm totally mistaken. I was on Integration_2.4.0 and thought I was on master. And I mistakenly pushed two commits I shouldn't have. Reverting.

Member

floam commented Nov 1, 2016

Er, I'm totally mistaken. I was on Integration_2.4.0 and thought I was on master. And I mistakenly pushed two commits I shouldn't have. Reverting.

@faho faho added the release notes label Dec 10, 2016

elasticdog added a commit to elasticdog/dotfiles that referenced this pull request Feb 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment