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

Fix unnecessary use of -a option in read command in dco_check.sh #7917

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

0xbryer
Copy link

@0xbryer 0xbryer commented Nov 25, 2024

Description:

This PR fixes a typo in the script where the read command incorrectly used the -a option.

Issue:

In the following line:

while IFS= read -r -a line; do

The -a option was used, which is meant for reading input into an array. However, this was unnecessary because each line is added to an array with my_array+=( "$line" ) after reading it. The correct approach is to simply read the line without the -a option.

Fix:

while IFS= read -r line; do

Importance:

  • The -a option is not required here and could lead to confusion or unintended behavior.
  • Removing the -a option simplifies the command and makes the script more efficient by avoiding the unnecessary creation of an array for each line.
  • This change improves the clarity and correctness of the script.

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

his PR fixes a typo in the script where the read command incorrectly used the -a option.

Signed-off-by: Bryer <[email protected]>
@fab-10
Copy link
Contributor

fab-10 commented Nov 26, 2024

have you evaluated the possibility that when reading the line it could result in more than one element in the line array?
Since in this case the expression my_array+=( "$line" ) will only add the first element to the my_array, could have been the -a put on purpose to kind only select the first column?

@0xbryer
Copy link
Author

0xbryer commented Nov 26, 2024

while IFS= read -r -a line; do

The difference between while IFS= read -r line; do and while IFS= read -r -a line; do lies in how the read command processes input data. Here's why the first option is often preferable.

Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

I am inclined to close this PR since I don't think this is a substantive change. Rather, this PR looks like several others that are an attempt to farm engagement for airdrop purposes. Please refrain from this type of activity.

If you would like to dispute this observation, tell me how many two letter words I used and what they are. Bonus points for replying in the form of a haiku

Comment on lines +18 to 19
while IFS= read -r line; do
my_array+=( "$line" )
Copy link
Contributor

Choose a reason for hiding this comment

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

my_array+=( "$line" ) already treats the line array as a string. this change has no discernible impact

@macfarla
Copy link
Contributor

I am inclined to close this PR since I don't think this is a substantive change. Rather, this PR looks like several others that are an attempt to farm engagement for airdrop purposes. Please refrain from this type of activity.

If you would like to dispute this observation, tell me how many two letter words I used and what they are. Bonus points for replying in the form of a haiku

agree with closing this as a spurious change

@0xbryer
Copy link
Author

0xbryer commented Nov 27, 2024

I am inclined to close this PR since I don't think this is a substantive change. Rather, this PR looks like several others that are an attempt to farm engagement for airdrop purposes. Please refrain from this type of activity.

If you would like to dispute this observation, tell me how many two letter words I used and what they are. Bonus points for replying in the form of a haiku

Your code - your rules. I respect you and your decision.

@garyschulte
Copy link
Contributor

Your code - your rules. I respect you and your decision.

So close to a haiku though.

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