Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

autogen.sh shell script rewrite #372

Closed
wants to merge 1 commit into from
Closed

autogen.sh shell script rewrite #372

wants to merge 1 commit into from

Conversation

tricantivu
Copy link
Contributor

@tricantivu tricantivu commented Oct 9, 2023

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
Copy link
Contributor

guijan commented Oct 9, 2023

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

@tricantivu
Copy link
Contributor Author

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

Should I rebase my commits?

@tricantivu tricantivu marked this pull request as ready for review October 15, 2023 00:54
@guijan
Copy link
Contributor

guijan commented Oct 15, 2023

Should I rebase my commits?

Yes, break lines before they go over 80 columns.

@N-R-K N-R-K closed this in bd565cc Oct 21, 2023
@N-R-K
Copy link
Collaborator

N-R-K commented Oct 21, 2023

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

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.

3 participants