Vencord / Installer

A cross platform gui/cli app for installing Vencord
GNU General Public License v3.0
516 stars 83 forks source link

Use test for conditional #141

Closed unrealapex closed 2 months ago

unrealapex commented 2 months ago

Use test for the privilege escalation logic instead of running commands and redirecting stdout.

Vendicated commented 2 months ago

?

unrealapex commented 2 months ago

Test is redundant for this. My apologies!

Vendicated commented 2 months ago

the current code checks for a non error exit code which is the recommended and most robust way. your PR changes it to check for whether the command prints anything, which is a very bad way of doing it and could easily lead to false positives if some implementation printed 'command not found'

before trying to make further contributions, please consider whether your contributions are actually productive. there is little point changing code that is not broken. prs should add (good) features or fix bugs, not refactor random code

unrealapex commented 2 months ago

the current code checks for a non error exit code which is the recommended and most robust way.

I had not remembered that, I had made the assumption that using command substitution was the de facto way to compare exit codes. Thank you for pointing that out.