Well, this started out in the spirit of moving the go pathmunge wrapper logic to core, but as @davidpfarrell points out, in my haste I forgot that for each go path, I have to append something. So I'll leave this PR in case y'all want it, but I'm cool with closing it too. Let me know what you think! I'm not in a rush on this one.

Thanks for working through this! So is it OK to merge this as-is? I've seen that you made changes to pathmunge to support multiple path segments and added more test cases - OK to merge this? And then you address the go items at a later stage?

Yeah. I'll add that negative test and address go in a follow-up PR.

Well, this started out in the spirit of moving the go pathmunge wrapper logic to core, but as @davidpfarrell points out, in my haste I forgot that for each go path, I have to append something. So I'll leave this PR in case y'all want it, but I'm cool with closing it too. Let me know what you think! I'm not in a rush on this one.

Considering how much effort you put into fleshing out the tests, I'd be inclined to keep this PR moving.

local i=${#a[@]}
    while [[ $i -gt 0 ]] ; do

Regarding walking the input list backwards:

So pathmunge "path1:path2" "before" would yield:

path1:path2:${PATH}

But pathmunge "path1:path2" "after" would yield:

${PATH}:path2:path1

So an interesting question arises:

Should processing pathmunge "path1:path2" ("before"|"after") be essentially the same as

pathmunge "path1" ("before"|"after")
pathmunge "path2" ("before"|"after")

Or should the resulting path contain "path1:path2" in the same order as given? ie:

pathmunge "path1:path2" "before" yeilds path1:path2:${PATH}
while
pathmunge "path1:path2" "after" yields ${PATH}:path1:path2

I'm slightly inclined to think that the relative order should be maintained, as the user is likely expressing a preference to have "path1" present before "path2" in the resulting path.

Implementing this would require walking the list backward in the "before" case, but walking it forward in the "after" case.

Maybe we can check for the "after" case and simply reverse the list before walking it (vs having to keep track of which direction we're moving)?

A quick googling didn't show any built-in function to reverse a list, so maybe setting up $start, $increment and $stop could allow you to generically traverse the list. the seq command might be handy here.

[edit: Updated for brevity, typos]

I would remove the function keyword here:

This works in some shells, but not in others. You should never combine the keyword function with the parentheses () when defining a function.

Bash (at least some versions) will allow you to mix the two. Most of the shells won't accept that (zsh 4.x and perhaps above will - for example). Some shells will accept function foo, but for maximum portability, you should always use:

foo() {
  ...
 }

Reference

@ahmadassaf I tend to target bash specifics intentionally because this is a bash specific project. I'll take a look at this suggestion though.

Edit: I would like to see an issue that tracks this pattern and purges it from the repo instead of addressing it at each PR. Contributors are going to base their new work on what is existing, and we mix every form possible in this project.

Indeed - we have that pattern function .*\( (use this as a regexp for searching) all over the code base, more than 200 times.

We should probably address that from a general perspective and document it in the contribution guidelines. @ahmadassaf - care to open an issue for that?

Can do indeed .. also I have been working on a fork where I did a large-scale refactoring (but also introduced some very opinionated stuff in there).

I can share that once am done these days and can happily cherry pick and move some stuff over in a series of PRs. the changes addresses some of the common bash pitfalls, ensure code style consistency in things like comments, function definitions, etc.

I have not finished everything yet but you can have a look at where I am at now https://github.com/ahmadassaf/gaudi-bash

Will be updating docs today but still need to work on themes and custom components some time later this week/weekend.

I am by no means a bash expert but tried my best to work something that made sense to me, so I hope some of these decisions make sense to you as well :)

The main changes in this repo are:

• Split the core engine from the components (plugins, aliases, completions and themes to be done soon)
• Split the code into smaller set of files e.g., components, helpers, utils, etc.
• Ensure consistent way of writing bash code (function definitions, comments, etc.)
• Following as much as possible bash best practices
• Make sure to comply with shellcheck as much as possible
• Ensure high coverage unit tests for all core functions and split test functions as well into small units
• Merged some of the work from bash-it issues and openPRs that made sense to me

I have not finished everything yet but you can have a look at where I am at now https://github.com/ahmadassaf/gaudi-bash

Will be updating docs today but still need to work on themes and custom components some time later this week/weekend.

I am by no means a bash expert but tried my best to work something that made sense to me, so I hope some of these decisions make sense to you as well :)

The main changes in this repo are:

* Split the core engine from the components (plugins, aliases, completions and themes to be done soon)

* Split the code into smaller set of files e.g., components, helpers, utils, etc.

* Ensure consistent way of writing bash code (function definitions, comments, etc.)

* Following as much as possible bash best practices

* Make sure to comply with shellcheck as much as possible

* Ensure high coverage unit tests for all core functions and split test functions as well into small units

* Merged some of the work from bash-it issues and openPRs that made sense to me

Wow, very impressive!
You seem to put many hours into this, and I think that most things can be merged into the mainline.

I think that the best way for us to go through all of this effort is for you to cherry-pick specific changes and submit them as PRs here. This way we can make small incremental changes.

Your opinions @nwinkler @cornfeedhobo ?

I have not finished everything yet but you can have a look at where I am at now https://github.com/ahmadassaf/gaudi-bash

HI All - This line of conversation is off track from the purpose of this PR.

@ahmadassaf I recommend you create an issue to continue discussions on the work you're doing. Name the issue something like "Discuss: My Work on a Bash-It Factor and Possible Path to Integration"

You can use your post above as the starting post of the issue.

I look forward to further discussions on your efforts, but let's keep comments on this PR focused on "support multiple paths in pathmunge"

Thanks!

Thank you @davidpfarrell and @NoahGorny. I opened #1678 and looking forward to hearing your thoughts in there

@davidpfarrell I got to thinking about this again and you seem interested, so I'm looking to bounce thoughts off you:

I closed this for the time being because the second parameter is currently optional. I was thinking that a third option, which would require the inclusion of the second option as well, wouldn't be elegant. But here's the examples I wrote out and I'd like to know what you think about this.

    param '1: path, or colon separated list of paths, to add to PATH.'
    param '2: after|before, if unset, defaults to after. required if setting a suffix (param 3).'
    param '3: (optional) suffix to append to each entry.'
    example '`pathmunge /path/to/dir` is equivalent to PATH=/path/to/dir:$PATH'
    example '`pathmunge /path/to/dir after` is equivalent to PATH=$PATH:/path/to/dir'
    example '`pathmunge /path/one:/path/two` is equivalent to PATH=/path/one:/path/two:$PATH'
    example '`pathmunge /path/one:/path/two after` is equivalent to PATH=$PATH:/path/one:/path/two'
    example '`pathmunge /path/one:/path/two before /bin` is equivalent to PATH=/path/one/bin:/path/two/bin:$PATH'

Hi @cornfeedhobo,

here's the examples I wrote out and I'd like to know what you think about this.

My first thoughts are that its interesting, and since its an optional 3rd parameter, it shouldn't break any existing functionality.

My next thoughts are: What is/are the use-cases for this? Some that come to mind are:

• I didn't create the new path entry variable (or I could have appended the suffix a that time)
• I have multiple (or possibly multiple) paths in the variable (or i could append the suffix to the variable in my path-munge call)
• I didn't know what the suffix would be while I was building the (possibly multiple) path variable

Additionally, we have:

• I'm sure that each of the (possibly multiple) paths in the variable require the same suffix

I'd be interested in knowing/seeing a code example where delaying the suffix appending is either a requirement or a significant convenience.

I don't think the additional code needed in the pathmunge function would be too much, but it might make the tests for the function noticeably more complex.

Hope that helps!

@davidpfarrell The first case would be the go plugin. It currently has a pathmunge wrapper handling this.

Honestly, at this point, I've been trying to make bash-it happy for a week and need to get back to golang code, so I'm happy to scrap this entire train of thought and just leave pathmunge as it is, because I can't think of another real-world use case, only the same abstract ones you thought of.

@davidpfarrell Yeah, I'm going to scrap this. Just not enough value for the time cost. Thanks again for taking a look.