Skip to content

Use test for conditional#141

Closed
unrealapex wants to merge 1 commit into
Vencord:mainfrom
unrealapex:conditional
Closed

Use test for conditional#141
unrealapex wants to merge 1 commit into
Vencord:mainfrom
unrealapex:conditional

Conversation

@unrealapex
Copy link
Copy Markdown

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

@Vendicated
Copy link
Copy Markdown
Member

?

@Vendicated Vendicated closed this Sep 14, 2024
@unrealapex
Copy link
Copy Markdown
Author

Test is redundant for this. My apologies!

@Vendicated
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants