Replies: 2 comments
-
|
Beta Was this translation helpful? Give feedback.
0 replies
-
|
Beta Was this translation helpful? Give feedback.
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
Part 1 is here: #1
Both head(1) and sed(1) here aren't needed. By default, the
read
builtin will already read the first line of its input, so there's your head(1) functionality sorted. As for sed(1), you can easily trim leading and trailing whitespaces with a parameter expansion's prefix and suffix removal, respectively. IE:I used pipes to demonstrate the string barriers. I realise it might seem a bit unwieldy, but that, plus using just
read
to replace head(1), it's a more efficient use of system resources and you won't require sed(1).Ideally, you'd:
Or use the
[[
keyword with the=~
REGEX comparison operator and the$BASH_REMATCH
array variable, but neither solution seemed to actually work, unfortunately. I'm not sure why. Perhaps I was having a blonde moment. The more manual approach works very well though.https://github.com/The-Repo-Club/proctl/blob/4cd9e91dc413b720ff34579151617867298cebe8/proctl#L209
Again, you're using a substring unnecessarily, and what's more, is that you're probably exiting from the substring, making the exit status used here meaningless.
Are you trying to group commands together? You probably mean to use braces, like this:
Notice how grep(1) matches both 'Test' lines? This approach groups commands together, which is very useful, but without using a subshell. Unnecessarily using subshells is inefficient. Just ask the Borg! :P
https://github.com/The-Repo-Club/proctl/blob/4cd9e91dc413b720ff34579151617867298cebe8/proctl#L231
Again, the
double negativesconfusing logic here need to be addressed. Refer to my first issue for one of my posts which explains this problem.https://github.com/The-Repo-Club/proctl/blob/4cd9e91dc413b720ff34579151617867298cebe8/proctl#L261-L265
Again, using a subshell unnecessarily, when you probably want to just group them together with braces. Although, in this case, I recommend that you just an
if
statement. To some extent, it's a stylistic choice, so it's up to you.https://github.com/The-Repo-Club/proctl/blob/4cd9e91dc413b720ff34579151617867298cebe8/proctl#L261-L265
You can replace tr(1) and head(1) here with something like:
BTW, that's an example of an unavoidable "double-negative", but it's still quite clear, I think. However, I've just realised I keep saying "double-negative", when that's not at all what this is. I'm not sure what an appropriate term would be, other than "confusing logic". Lol Hopefully you know what I mean, though.
https://github.com/The-Repo-Club/proctl/blob/4cd9e91dc413b720ff34579151617867298cebe8/proctl#L261-L300
You can avoid needing basename(1) by using parameter expansion's prefix removal, to clear everything up to and including the last '/' in a path. IE:
https://github.com/The-Repo-Club/proctl/blob/4cd9e91dc413b720ff34579151617867298cebe8/proctl#L327
I'm not sure why you've written it in this way.
:-
means "set this to this if null", but you've chosen to set$EDITOR
to null if it's null, which is a bit strange. Did you forget to finish that line? Lolhttps://github.com/The-Repo-Club/proctl/blob/4cd9e91dc413b720ff34579151617867298cebe8/proctl#L334
I don't have and don't entirely know what
fzf
is, so I can't help much here, but I can say that I'm confident awk(1) and sed(1) can probably be replaced with a BASH approach here. I'm not sure how viable that is though, because I haven't got the original data andfzf
. Remember, BASH's[[
keyword supports REGEX matching via=~
, and remember that$BASH_REMATCH
array variable is a thing for backreferencing.You do this sort of thing a lot, I've noticed, which means there's probably a great opportunity for a function. You've got plenty of functions for the sake of functions, but not many functions for the sake of actually avoiding repetition and to make your code more flexible. To me, that's like buying car after car, but never ever driving them.
For example,
search_license()
,show_license()
,language_picker()
,get_language()
, andinitialize_repo()
are used only once, so they're somewhat redundant. I realise there's a level of stylistic license here, but I can't not mention it. Lol Your approach would make more sense in C, I imagine.However, if it truly helps you program effectively, then ignore me.
https://github.com/The-Repo-Club/proctl/blob/4cd9e91dc413b720ff34579151617867298cebe8/proctl#L365-L367
I'm not entirely sure why you've used awk(1) here, because, if it were needed, it'd break the
depends
call anyway. Actually, I'm pretty sure a space in a filename is not allowed or is particularly non-standard in$PATH
. Am I missing something here?https://github.com/The-Repo-Club/proctl/blob/4cd9e91dc413b720ff34579151617867298cebe8/proctl#L512
Unfortunately, due to the way you've checked for dependencies (when they're needed), it's gonna be a massive pain if someone is missing a bunch. They'll run the program, be told they need something, likely in the middle of it actually doing a bunch of stuff, which is potentially problematic, then they correct that, only to run the program and find they're missing something else! Check any one of my programs (AE, for example) and you'll see that I handle all dependencies first, before I do anything that requires an external tool. There's almost always a method to my madness. Lol
There are other things I could point out, but I'd pretty much be repeating myself, and I need a break; maybe I need a function for this. ;)
I think that's it done. Overall, I think you're doing really well. :) Give me a shout if you need me to look at anything else. Also, sorry this didn't make it to a video, if that's what you had in mind. In retrospect, I probably should've just done that. Lol
Beta Was this translation helpful? Give feedback.
All reactions