Skip to content

Conversation

@Devjiu
Copy link
Contributor

@Devjiu Devjiu commented May 22, 2024

Remove debug messages. There are still possible misuse of script as options without "-" in the start will be ignored.

Copy link
Contributor

@leshikus leshikus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not using -e is a bad practice; moreover, -e should be set at the script start with set -e

I don't like removing it

If you have a command cmd, which could potentially fail, and you want to handle that, use if cmd or cmd ||

@Devjiu Devjiu force-pushed the dmitriim/script_remove_debug_prints branch 2 times, most recently from f832a99 to 68830e0 Compare May 23, 2024 11:27
@Devjiu
Copy link
Contributor Author

Devjiu commented May 23, 2024

Not using -e is a bad practice; moreover, -e should be set at the script start with set -e

I don't like removing it

If you have a command cmd, which could potentially fail, and you want to handle that, use if cmd or cmd ||

I've reworked this part based on Andrey's comments, now the options parser is a little simpler and I've removed the bash syntax since it's not needed yet.

Copy link
Contributor

@leshikus leshikus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

Remove debug messages. There are still possible misuse of script
as options without "-" in the start will be ignored.
@Devjiu Devjiu force-pushed the dmitriim/script_remove_debug_prints branch from 68830e0 to c99d0ae Compare May 23, 2024 12:52
@Devjiu Devjiu merged commit 4b860f9 into main May 23, 2024
@Devjiu Devjiu deleted the dmitriim/script_remove_debug_prints branch May 23, 2024 12:56
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.

4 participants