search

actuallymentor / battery

CLI/GUI for managing the battery charging status for Apple silicon (M1, M32, M3) Macs
MIT License
3.6k stars 148 forks source link

battery allows target above 100% #72

Closed earthsound closed 1 year ago

earthsound commented 1 year ago

What is the issue? (required) battery allows setting a target above 100%

What exactly did you do to produce the issue? (required) Steps to reproduce the behavior: run:

battery maintain 800

result:

Starting battery maintenance at 800%
Maintaining battery at 800%

Expected behavior (required) I assume battery should limit the max target to 100% and inform user. Also, instead of just generating an error and keeping the prior setting, the target gets set to the max (100%).

matomatical commented 1 year ago

Related (sorry if this should be a separate issue).

run:

battery maintain off

(I meant 'stop', oops)

result:

Starting battery maintenance at off%
Maintaining battery at off%

(battery stops charging even when plugged in at around 20%)

expected:

error message (suggest it include "perhaps you meant 'stop'?")

actuallymentor commented 1 year ago

Nice catches. You comfortable doing a pull request?

matomatical commented 1 year ago

To fix the issues identified above, is an easy PR: The code on lines 384 to 418 of battery.sh is responsible for the maintain action. Just add some checks, perhaps clip numbers to valid range as suggested in OP, and show an error message if the input is fully nonsense.

That will leave a related class of input validation bugs not mentioned, but that now become clear. The other actions also lack input validation. For example, battery charging neither-on-nor-off will stop maintenance but neither enable nor disable charging.

That suggests a slightly longer PR: Implement ad-hoc input validation and error messages for all the commands.

Ultimately, I think it would be better to consider a third, deeper PR: Implement a unified, maintainable system for input validation for the whole CLI. Such a change would require some familiarity with bash and/or tools such as getopt/getopts.

I don't know it's really worth doing the easier PRs and I personally don't have time for the deeper one right now, sorry.