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

Minor coding style changes and typo fixes #1

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

Conversation

likewhoa
Copy link
Contributor

@likewhoa likewhoa commented Jan 5, 2014

  1. Switched to BASH Arithmetic expressions for some if statements that used numbers.
  2. Added ${var} as best practice
  3. Minor spelling fixes

1. Switched to BASH Arithmetic expressions for some if statements that used numbers. 
2. Added ${var} as best practice
3. Minor spelling fixes
Example: ./datawatch.sh xpool jump
(xpool - dtc.xpoll.xram.co | gpool - dtc.gpoool.net)
WARNING: you have to edit script if you haven't do so.
USAGE
Copy link
Owner

Choose a reason for hiding this comment

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

exit got lost here i guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a much better way to print multiple lines, instead of having all those echos :D

Copy link
Owner

Choose a reason for hiding this comment

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

Still no exit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I missed the exit

arkhebuz added a commit that referenced this pull request Jan 6, 2014
@arkhebuz
Copy link
Owner

arkhebuz commented Jan 6, 2014

Hmm, what am I doing here...
Didn't want to merge with master so I created new branch, then cherry-picked your commit from there and tried to fix the above. There's probably better way around somewhere... ...outside my github skills.

Could you explain why using ${var} everywhere is best practice? I know there are situations where they are clearly needed (like arrays).

Thanks for noticing the script and writing the patch, though!

@likewhoa
Copy link
Contributor Author

likewhoa commented Jan 7, 2014

@arkhebuz ${var} habit for me really, but allot of people in #bash can agree on making a habit in ${var} and or "${var}"

@arkhebuz
Copy link
Owner

arkhebuz commented Jan 7, 2014

As far as Bash practices go, I would call quotation marks mandatory if you don't want end up like bumblebe.

About these DNS servers, I checked that with echos like below to be sure.

declare -A dns_serversA=( [google2]=8.8.4.4 [opendns1]=208.67.222.222 [level3]=209.244.0.3 [comodo]=8.26.56.26 [google1]=8.8.8.8 )
dns_servers=(8.8.8.8 8.8.4.4 208.67.222.222 209.244.0.3 8.26.56.26)
echo "\${dns_servers[@]}         ${dns_servers[@]}"
echo "\${dns_servers[@]:1:4}     ${dns_servers[@]:1:4}"
echo "\${dns_servers[google1]}   ${dns_servers[google1]}"
echo ""
echo "\${dns_serversA[@]}         ${dns_serversA[@]}"
echo "\${dns_serversA[@]:1:4}     ${dns_serversA[@]:1:4}"
echo "\${dns_serversA[google1]}   ${dns_serversA[google1]}"

And this gives me:

${dns_servers[@]}         8.8.8.8 8.8.4.4 208.67.222.222 209.244.0.3 8.26.56.26
${dns_servers[@]:1:4}     8.8.4.4 208.67.222.222 209.244.0.3 8.26.56.26
${dns_servers[google1]}   8.8.8.8

${dns_serversA[@]}         208.67.222.222 8.8.4.4 8.8.8.8 8.26.56.26 209.244.0.3
${dns_serversA[@]:1:4}     208.67.222.222 8.8.4.4 8.8.8.8 8.26.56.26
${dns_serversA[google1]}   8.8.8.8

So I need to fix the fix of a patch.

@arkhebuz
Copy link
Owner

arkhebuz commented Jan 8, 2014

OK, if you have something more to comment here, feel free to do so. I will merge the second branch (based on your modified patch) with master soon, then close this request.

Little long discussion for such small piece of code... 100 sloc in Bash - serious business.

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.

None yet

2 participants