Tidy up add_to_list in manage

[trn1ty]

While looking at the add_to_list function in manage I noticed the function could probably be implemented as a single shell pipeline, so I went ahead and did that. Instead of bouncing local variables around and using Bash features to sculpt everything before spitting it out, this leans on sed(1p) to chip away at a stream as it slowly forms into the desired output.

[Botspot]

What is the goal of this PR?

• performance
• code readability
• code robustness
• future maintainability
• minimizing lines of code
• minimizing external program calls

To me this may be more "tidy", but my gut reaction is that it would perform more slowly as it seems to be using sed more and internal shell builtins less. (echo, test, printf, eval are built-in to bash and are generally much faster than repeated calls to external programs like sed, awk, or grep)
Also we have inconsistent usage of the input variables throughout the function, sometimes using $action, other times using $1. Not sure what that does except force a developer to memorize what $1, $2, $3, and $4 mean to comprehend what is going on here.

[Botspot]

Regarding code robustness, I see a few places where sed is given an app name as an argument. This could be problematic for app names with special characters in them. These are the special characters that some app names are using now:
( ) . + - '
And there could be more in the future.
These app names could cause sed to perform strange regex edge cases. This all would need to be rigorously tested.

[theofficialgman]

I find these changes overall much harder to read and they make it more difficult to extend the functionality of the function while "seemingly" providing no other benefit.

If you could respond to botspots first comment that would help.

[trn1ty]

These app names could cause sed to perform strange regex edge cases. This all would need to be rigorously tested.

I didn't consider this, I'll have to figure that out!

my gut reaction is that it would perform more slowly as it seems to be using sed more and internal shell builtins less

That was something I was keeping in the back of my mind when making these changes. My goals tend to lean more towards portability than performance but performance is a very valid concern. In this particular case, the performance losses from process spawning are offset by the function being basically one giant pipeline, so all the processes are (as far as I know) spawned at once at the start of the pipe instead of being spawned as the function progresses (which would have happened previously).

Also we have inconsistent usage of the input variables throughout the function, sometimes using $action, other times using $1. Not sure what that does except force a developer to memorize what $1, $2, $3, and $4 mean to comprehend what is going on here.

My intent was to make the function so short the programmer could keep the comment explaining the positional arguments in the same view of the code, so having named arguments would no longer be necessary. $action is the relevant positional argument, but with -file trimmed off if the action was update-file (so the tense modification in the text still makes sense), but that's unclear in my changes and kind of an ineligant solution I'm going to have to think about.

[theofficialgman]

still need this answered:

What is the goal of this PR?

• performance
• code readability
• code robustness
• future maintainability
• minimizing lines of code
• minimizing external program calls

[theofficialgman]

Also we have inconsistent usage of the input variables throughout the function, sometimes using $action, other times using $1. Not sure what that does except force a developer to memorize what $1, $2, $3, and $4 mean to comprehend what is going on here.

To add to this, we generally prefer to rename the input arguments on purpose so that developers don't need to memorize the input argument order. If you look at most of the other functions in pi-apps where an input argument is used multiple times in the context of a function, we do this input argument renaming.

It is also our standard to define the use of the function in only one line before the function or inline with the function name with the input arguments defined within the function context, not before like you have done here.

This PR really only changes this function to not abide by these standards while providing no other value. For now I will close it.