resurrecting-open-source-projects / scrot

SCReenshOT - command line screen capture utility
Other
495 stars 49 forks source link

autogen.sh shell script rewrite #372

Closed tricantivu closed 12 months ago

tricantivu commented 1 year ago

I have seen the pull request #174 by user @guijan, however, I disagree with the code style and the argument against using echo.

I believe it is easier to validate the autogen.sh shell script command line arguments and, albeit more readable by just expanding the first positional parameter and ignoring the rest. Furthermore, the autogen.sh shell script from #174 would consider an empty string as valid (e.g., ./autogen.sh '').

If the script were to become more complex, arguments can be parsed in other ways.

Regarding the use of the echo command, there should not be any significant portability issue because all strings or arguments do not contain escape sequences to be interpreted. When more formatting is required, echo can be replaced to prevent errors because of the differences in their options. For more information:

While editing the autogen.sh shell script I noticed there was a backslash near the end of a line with an rm command. Is there some line length limit that I am not aware of?

guijan commented 1 year ago

The backslash is to avoid going over 80 columns in a line.

tricantivu commented 1 year ago

The backslash is to avoid going over 80 columns in a line.

Should I rebase my commits?

guijan commented 1 year ago

Should I rebase my commits?

Yes, break lines before they go over 80 columns.

N-R-K commented 12 months ago

Thanks for the PR, I've went with something simpler based on #174.