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(git_commit): Add only_detached option #738

Merged
merged 3 commits into from Feb 12, 2020

Conversation

ybc37
Copy link
Contributor

@ybc37 ybc37 commented Dec 15, 2019

Description

This adds a new option only_detached to the git_commit module. It doesn't change default behavior as its default value is false. Set to true, it will only show the commit hash if the repo is in detached HEAD state; if HEAD is on a branch, it will not output the hash. Also see #706.

Motivation and Context

Closes #706

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Screenshots (if appropriate):

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.

@ybc37 ybc37 force-pushed the feat/git-commit-only-detached branch from a885fc6 to 9d77904 Compare December 16, 2019 08:04
Copy link
Member

@matchai matchai left a comment

Choose a reason for hiding this comment

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

Sorry for not getting to this PR sooner. 😅

This feature looks great! In fact, I think it should be the default behavior for this module.

I'd be in favor of enabling the git_commit module by default if only_detached is true by default:

image

Thoughts @starship/astronauts? 👩‍🚀👨‍🚀

@jletey
Copy link
Member

jletey commented Dec 20, 2019

@matchai I think that this is a splendid idea! And very useful too! Ship it 🚀

@ybc37
Copy link
Contributor Author

ybc37 commented Dec 21, 2019

I'm happy to get it merged :) Just let me know if I should change the defaults once there's a consensus or feel free to change it yourself, of course :)

OT: Other prompts have this combined. If on branch show master, if detached only show hash ba18e1a (instead of HEAD). So basically merge the two modules git_branch and git_commit. But I understand this doesn't work for people, who prefer to always have the hash in the prompt. Or would it be an option to show the branch and the hash in one module?

@jletey jletey changed the title feat: Add only_detached option to git_commit module (#706) feat: Add only_detached option to git_commit module Dec 21, 2019
@jletey jletey changed the title feat: Add only_detached option to git_commit module feat(git_commit): Add only_detached option Jan 13, 2020
@matchai
Copy link
Member

matchai commented Feb 6, 2020

Unfortunately this PR got lost in the mix.
I would still love to see this merged in, but testing it locally, it seems to be showing the hash when I'm on master.

Can anyone else verify that it's working for them?

@ybc37
Copy link
Contributor Author

ybc37 commented Feb 6, 2020

It works here, just checked. Have you set only_detached = true for the git_commit module? Default behavior is still to always show the hash, but as said before, just let me know if I should change that.
(Also, I forced pushed (maybe a day) after submitting this PR to fix a logical bug, so make sure to be up-to-date 😉)

Edit: I also rebased on master right now.

@ybc37 ybc37 force-pushed the feat/git-commit-only-detached branch from 9d77904 to 452ea5b Compare February 6, 2020 19:15
@matchai
Copy link
Member

matchai commented Feb 6, 2020

There we go! Yep, looked like a blunder on my end. 😅
Everything looks like it works well. 👍

Could you please set the default behavior to the following:

disabled = false
only_detached = true

You'll want to update the config, docs, and remove these lines here:

if config.disabled {
return None;
};

@ybc37
Copy link
Contributor Author

ybc37 commented Feb 7, 2020

Sure, I changed it :)

Maybe someone can have a look at the commit. Especially the tests, if it's ok this way, or needs more work (I only apply the minimal changes to the tests, but maybe we want to change them, so they reflect the default better?).

About the line you suggested to remove: I'm not sure if understand. Isn't that still needed if someone wants to disable the module?

@matchai
Copy link
Member

matchai commented Feb 7, 2020

About the line you suggested to remove: I'm not sure if understand. Isn't that still needed if someone wants to disable the module?

That line was there due to us not yet implementing logic to handle disabled-by-default modules. We do, however, have logic to handle manually disabled modules via configuration here: https://github.com/starship/starship/blob/master/src/context.rs#L106-L113

@ybc37 ybc37 force-pushed the feat/git-commit-only-detached branch from 2f23564 to 5edfe88 Compare February 7, 2020 22:06
@ybc37
Copy link
Contributor Author

ybc37 commented Feb 7, 2020

Ok, I see. Thanks for explaining! :)

Removed that and rebased the branch on master.

Copy link
Member

@matchai matchai left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍
Thank you for seeing this PR through. 😊

I went ahead and removed the updates to the localized docs since those are automatically done by Crowdin.

@matchai matchai merged commit 0312c7b into starship:master Feb 12, 2020
dagbrown pushed a commit to dagbrown/starship that referenced this pull request Oct 22, 2021
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.

[Feature/git_commit] Only show git commit hash in detached HEAD state
3 participants