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: refactor to bash compatibility #18

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ndom91
Copy link

@ndom91 ndom91 commented May 5, 2022

Why did you make this change

Make it bash compatible and further the reach of your great extension!

Feel free to close if you're a big zsh fan and explicitly want to keep it this way.. Otherwise I think it'd be wise to make this more widely compatible! 🎉

What have you done in this PR

  • Refactored a few high-level incompatible statements, like the array manipulation
  • Formatted with shfmt and also fixed all shellcheck linting errors (other than 2207 which is disabled file-wide)

What was left to do

Dependencies

- zsh

Checklist:

  • My code follows the style of this project
  • I have performed a self-review of my own code
  • I have have tested my code locally to prove my fix is effective or that my feature works
  • I have made corresponding changes to the README.md

@waldyrious
Copy link
Contributor

Awesome work, @ndom91! 👏👏 For the record, this was first raised in #2, and initial POSIX-ifying work was done in #3.

@ndom91
Copy link
Author

ndom91 commented May 6, 2022

Awesome work, @ndom91! 👏👏 For the record, this was first raised in #2, and initial POSIX-ifying work was done in #3.

Ah I hadn't even seen those haha. Thanks for the initial work then!!

@davidraviv
Copy link
Owner

Thanks for the great contribution, @ndom91!
I believe that relying on bash will allow more developers to enjoy this extension.

This is a significant change. It will take some time to review and test it.

I have one concern, though. This change may break the extension for a developer who only has zsh installed. Though, I assume that it is a rare case.

@ndom91
Copy link
Author

ndom91 commented May 15, 2022

Thanks for the great contribution, @ndom91!
I believe that relying on bash will allow more developers to enjoy this extension.

This is a significant change. It will take some time to review and test it.

I have one concern, though. This change may break the extension for a developer who only has zsh installed. Though, I assume that it is a rare case.

Yeah I can double check and run it with Zsh as well. I have a macbook somewhere.. haha.

EDIT: Can confirm the array splitting doesn't work in zsh now.

image

See:

split() {
IFS=$'\n' read -d "" -ra arr <<<"${1//$2/$'\n'}"
printf '%s\n' "${arr[@]}"
}

local_branches=$(split "$local_branches_str" " ") # split string by " " to array

Will work on this shortly

EDIT 2: Note - https://unix.stackexchange.com/a/491459

@davidraviv
Copy link
Owner

@ndom91
Sorry for the delay in testing this.

Please take a look at a test I made that compares the results of the current code (1st run) to the PR code (2nd run).

The branch add-git-ignore exists both on the remote and local. Hence the branch is not expected to be deleted.

✅ The current code doesn't list the branch for deletion - as expected.
❌ The PR code lists the branch for deletion - not as expected.

You are welcome to take a look and push a fix. I promise to test it quickly 😉

Screen Shot 2022-05-23 at 17 46 39

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