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

theme/powerline: fix error scm: parameter not defined #2021

Merged
merged 1 commit into from
Jan 7, 2022
Merged

theme/powerline: fix error scm: parameter not defined #2021

merged 1 commit into from
Jan 7, 2022

Conversation

NariyasuHeseri
Copy link
Contributor

Fix bug that powerline theme's scm feature does not work.

Description

The commit 2991aa6 removed set +u
but still references the variable scm without defining it.
This commit remove the reference to scm.

Motivation and Context

This makes powerline theme's scm feature work again.

How Has This Been Tested?

Screenshots (if appropriate):

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 change)

Checklist:

  • My code follows the code style of this project.
  • If my change requires a change to the documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
  • I have added tests to cover my changes, and all the new and existing tests pass.

Copy link
Contributor

@gaelicWizard gaelicWizard left a comment

Choose a reason for hiding this comment

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

Looks like that may have been some vestige of a much older variable. I don't think it's defined anywhere, let alone anywhere near here.

Thanks for catching that!

@davidpfarrell
Copy link
Contributor

Hi all - Let's not merge this yet - I use this theme and have never received the error that would result from ${scm?} not being defined ... I want to dive into this a bit (maybe look at the revision history) and see how we got here ...

Thanks for bringing it up !

@NariyasuHeseri
Copy link
Contributor Author

Hi all - Let's not merge this yet - I use this theme and have never received the error that would result from ${scm?} not being defined ... I want to dive into this a bit (maybe look at the revision history) and see how we got here ...

Thanks for bringing it up !

Probably it's because you have set set +u in your .bashrc or by your bash installation?

Copy link
Contributor

@davidpfarrell davidpfarrell left a comment

Choose a reason for hiding this comment

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

OK!

Looks like this bug was introduced by @edubxb in #815 which was a nice refactor that enabled the configurable segment_prompt feature !

There is a scm function in base.theme.bash (link is version from time of #815)
Wonder if that ${scm} was a leftover WIP on the PR that was initially invoking the function directly?

Either way, I feel a lot more confident about removing it now, so thanks for holding off!

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

4 participants