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

"_command_exists" used in battery plugin but is not defined anywhere! #68

Closed
antofthy opened this issue Jul 16, 2019 · 7 comments · Fixed by #104
Closed

"_command_exists" used in battery plugin but is not defined anywhere! #68

antofthy opened this issue Jul 16, 2019 · 7 comments · Fixed by #104
Assignees
Labels
bug-report Issues about bug reporting help-wanted Issues that require help from third party P2 - Important Priority 2

Comments

@antofthy
Copy link

antofthy commented Jul 16, 2019

TLDR: command_exists() is defined in

command_exists () {
, but it's not used in https://github.com/ohmybash/oh-my-bash/blob/master/plugins/battery/battery.plugin.sh which expects _command_exists() where _ prefix is important.

Summarized by @Kreyren


Title basically says it all...

A function "_command_exists" is used in the battery plugin, but is not defined anywhere else in the repository.

... removed personal "cmd_found" equivalent function, designed for older shells ...

@antofthy
Copy link
Author

antofthy commented Jul 16, 2019

ASIDE: I also recommend breaking the percentage bar graph out to a separate utility.
Though it is an interesting (colorful) 5 character output.

I have found ahaving a script to generate a bar graph from a percentage is a very useful thing to have.
Example (could be more colorful)...
http://www.ict.griffith.edu.au/anthony/software/percent.sh.txt

@Kreyren Kreyren added P3 - Normal Priority 3 - Normal bug-report Issues about bug reporting help-wanted Issues that require help from third party P2 - Important Priority 2 and removed P3 - Normal Priority 3 - Normal labels Aug 15, 2019
@Kreyren
Copy link
Contributor

Kreyren commented Aug 15, 2019

Assigned P2 as bug.

Investigation needed -> Currently fix unknown.

@Kreyren
Copy link
Contributor

Kreyren commented Aug 20, 2019

@antofthy _command_exists where? (repository is using 3rd party code for completetion etc..)

@Kreyren Kreyren added P5 - Lowest priority Priority 5 - Lowest and removed P2 - Important Priority 2 labels Aug 20, 2019
@Kreyren
Copy link
Contributor

Kreyren commented Aug 20, 2019

Lowering priority untill info is provided

@antofthy
Copy link
Author

antofthy commented Aug 20, 2019

More information... Okay...

The plug-in "plugins/battery/battery.plugin.sh" used a function _command_exists multiple times (over a dozen). And yet a search of the whole repository did not turn up that function anywhere else, except its use in that plugin. It is simply not defined anywhere.

On the other hand in "lib/base.sh" a function "command_exists()" is defined!

command_exists () {
  type "$1" &> /dev/null ;
}

And in "lib/utils.sh" a simular function "type_exists()" is also defined...

type_exists() {
  if [ "$(type -P "$1")" ]; then
    return 0
  fi
  return 1
}

Which could be rewritten more concisely as...

type_exists() {
  [ "$(type -P "$1")" ]
}

Though checks if the given argument is an external command, and ignores functions of the same name. But its is used for same purpose.

In summery. The plug-in is broken. And in looking for a fix I found the two different functions being used for the same purpose. One probably should replace the other.

Kreyren pushed a commit to RXT067/what-the-bash that referenced this issue Aug 21, 2019
@Kreyren
Copy link
Contributor

Kreyren commented Aug 21, 2019

type_exists() was referenced in #103

contributions are welcomed since i'm trying to improve the code quality pasively (since it needs rewriting of lots of things)

EDIT: Note #73


About the command_exists() and _command_exists() it's stupid, but it doesn't create conflicts since they are different.

Edit: To clarify oh-my-bash should be able to source lib/base.sh for this

ping @nntoan How do you want to resolve conflicts alike?

Kreyren pushed a commit to RXT067/what-the-bash that referenced this issue Aug 21, 2019
@Kreyren
Copy link
Contributor

Kreyren commented Aug 21, 2019

@antofthy Referencing #104 if only command_exists() is the issue this should fix it.

@Kreyren Kreyren added P2 - Important Priority 2 and removed P5 - Lowest priority Priority 5 - Lowest labels Aug 21, 2019
nntoan pushed a commit that referenced this issue Feb 17, 2020
nntoan pushed a commit that referenced this issue Feb 17, 2020
pull bot pushed a commit to kis87988/oh-my-bash that referenced this issue Feb 17, 2020
pull bot pushed a commit to kis87988/oh-my-bash that referenced this issue Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-report Issues about bug reporting help-wanted Issues that require help from third party P2 - Important Priority 2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants