Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants