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

Add AbbreviateHomeDirectory setting #387

Merged
merged 3 commits into from
Feb 1, 2017
Merged

Add AbbreviateHomeDirectory setting #387

merged 3 commits into from
Feb 1, 2017

Conversation

shawmanz32na
Copy link

Adds a setting to optionally disable the home directory abbreviation in
the prompt. By default, this is set to true, preserving the existing
behavior.

Closes #386

Adds a setting to optionally disable the home directory abbreviation in
the prompt. By default, this is set to true, preserving the existing
behavior.
@@ -101,6 +101,8 @@ $global:GitPromptSettings = [pscustomobject]@{
DefaultPromptDebugSuffix = ' [DBG]$(''>'' * ($nestedPromptLevel + 1)) '
DefaultPromptEnableTiming = $false

AbbreviateHomeDirectory = $true
Copy link
Collaborator

@rkeithhill rkeithhill Jan 31, 2017

Choose a reason for hiding this comment

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

Can we change this to DefaultPromptAbbrevHomeDir or something with the DefaultPrompt prefix? Since this setting applies only to the posh-git default prompt.

Copy link
Author

Choose a reason for hiding this comment

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

After re-reading, this change only affects the default prompt, so sure.

Would DefaultPromptAbbreviateHomeDirectory be appropriate?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bit long but still not the longest property name. :-) Folder is a bit shorter than Directory but I guess it doesn't matter that much.

Copy link
Author

Choose a reason for hiding this comment

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

Given the option, I personally prefer 'Directory', so unless you feel strongly about using Folder to save the three characters, I'd prefer to use 'Directory'.

I'll change to DefaultPromptAbbreviateHomeDirectory, and you can veto as necessary.

@shawmanz32na
Copy link
Author

I looked through the test code, but didn't see any tests for the default prompt. Did I miss anything?

Creating a new test suite for the default prompt seems out of the scope of what I'm trying to accomplish here, so if I'm correct that there is not an existing test suite for this particular feature, I'd like to skip writing a test for this, since it's so simple, and since making this change in my posh-git installation results in the expected behavior ;-) .

@rkeithhill
Copy link
Collaborator

We just started adding Pester tests. I believe the built-in prompt could easily be tested. I'll look into adding some prompt tests tonight.

Pull request comments requested this change.
@shawmanz32na
Copy link
Author

^ It appears that I'm a dummy and fat-fingered the commit message.... I suppose I'll just hope you squash it when it gets merged in.

@rkeithhill
Copy link
Collaborator

Squash and merge isn't enabled for this repo but we'll get by with the typo. Let me think about the default for this setting a bit.

@@ -100,6 +100,7 @@ $global:GitPromptSettings = [pscustomobject]@{
DefaultPromptSuffix = '$(''>'' * ($nestedPromptLevel + 1)) '
DefaultPromptDebugSuffix = ' [DBG]$(''>'' * ($nestedPromptLevel + 1)) '
DefaultPromptEnableTiming = $false
DefaultPromptAbbreviateHomeDirectory = $true
Copy link
Owner

Choose a reason for hiding this comment

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

I probably sound like a broken record, but let's default to $false for folks who are used to posh-git uncustomized.

Bonus points if you find a place to mention this new setting in the README.

@dahlbyk
Copy link
Owner

dahlbyk commented Feb 1, 2017

^ It appears that I'm a dummy and fat-fingered the commit message.... I suppose I'll just hope you squash it when it gets merged in.

You are welcome to fix up the commit message and force push to this branch to update the PR before we merge. Or not, up to you what you want to be recorded for all time. 😀

@rkeithhill
Copy link
Collaborator

but let's default to $false for folks who are used to posh-git uncustomized.

Agreed. Not only posh-git but PowerShell uncustomized as well.

@dahlbyk dahlbyk merged commit b993be2 into dahlbyk:master Feb 1, 2017
@dahlbyk
Copy link
Owner

dahlbyk commented Feb 1, 2017

You are welcome to fix up the commit message and force push to this branch to update the PR before we merge.

Nevermind, I'm too eager to merge. Let the record show the misleading commit message is my fault.

@shawmanz32na
Copy link
Author

shawmanz32na commented Feb 1, 2017

Nevermind, I'm too eager to merge. Let the record show the misleading commit message is my fault.

It was a close race - I was force pushing as you were merging, but it appears I lost.

Did anyone find some time to get the base test written so I can add some more tests for this functionality?

@shawmanz32na shawmanz32na deleted the add-abbreviate-home-directory-setting branch February 1, 2017 18:20
@rkeithhill
Copy link
Collaborator

I'll handle the other setting and tests in a separate PR.

@shawmanz32na
Copy link
Author

Is there an opportunity for me to help, or am I just going to slow you guys down?

@rkeithhill
Copy link
Collaborator

I've got it. Besides the setting and tests there are several places of documentation I need to clean up / add info on this (and the other new setting) as well. BTW thanks for your PR and getting us thinking about how the default posh-git prompt should be configured.

@shawmanz32na
Copy link
Author

Alrighty. Cheers to you and the team for all the great work. Glad I could contribute a bit.

@dahlbyk
Copy link
Owner

dahlbyk commented Feb 1, 2017

Is there an opportunity for me to help, or am I just going to slow you guys down?

Sounds like @rkeithhill has this under control, but check out our up-for-grabs issues for other opportunities to contribute! 🎉 (And check http://up-for-grabs.net/ to find other projects looking for help!)

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

3 participants