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

Fix call to liquidprompt function #2175

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

DarrenBishop
Copy link

Missing leading underscore in _lp_escape

Description

Motivation and Context

Without this - presumably - the code fails silently, never presenting the branch name in the PS1 prompt.

How Has This Been Tested?

Forgive me if there are tests for themes, but I could not find any; I personally tested this by cd'ing into and out of Git clone directories with and without the fix.

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.

@davidpfarrell
Copy link
Member

davidpfarrell commented Nov 29, 2022

Relevant information from Version 2.0 Upgrade Notes (dated Oct 6):

_lp_escape()

Renamed to __lp_escape.
Return changed from stdout to $ret

Replace assignment statements like:

output="$(_lp_escape "$input")"

with:

local ret
__lp_escape "$input"
output=$ret

_lp_bzr_branch()

Return changed from stdout to $lp_vcs_branch

Recommended that _lp_vcs_branch is used instead.

Replace assignment statements like::

branch="$(_lp_bzr_branch)"

with::

local lp_vcs_branch
if _lp_bzr_branch; then
     branch=$lp_vcs_branch
fi

@davidpfarrell
Copy link
Member

Best I can tell this meets the requirements for the recent 2.0 version, but if we publish it as-is, isn't it going to break the existing installations?

Not sure what the best way to deal with it is?

Added `ret` to local declarations as per provided liquidprompt change notes
@DarrenBishop
Copy link
Author

@davidpfarrell I am on the case

@DarrenBishop
Copy link
Author

@davidpfarrell

Not sure what the best way to deal with it is?

It got a bit hairy at one point, but what I have done is:

Introduced some auxiliary functions

  • _lp_legacy to detect if <v2.y.z is in use by presence of the old _lp_escape
  • __lp_escape, but only if in legacy mode; it routes to __lp_escape and uses the ret return variable

These functions are unset before the sourcing of Liquidprompt, to provide a clean slate if reloading the shell etc.

Updated _lp_git_branch to use the lp_vcs_branch return variable, but also print to stdout and (importantly) return 0 when in legacy mode.

I think a good practice going forward would be to pin the version of Liquidpromt to a tagged version, maybe via some env var, and print a warning/disclaimer if it's ever changed from the default.

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