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

Custom DNS support and better codes #208

Closed
wants to merge 4 commits into from
Closed

Custom DNS support and better codes #208

wants to merge 4 commits into from

Conversation

sayem314
Copy link
Contributor

@sayem314 sayem314 commented May 1, 2018

I have added support for custom DNS and improved overall coding. Also fixed a DNS(9) bug.

@angristan
Copy link
Owner

Hello, and thank you for your PR.

Could you please explain what does read -rp enhances? What about || return?

Also if it doesn't bother you, could you do 2 separate PRs? 🙏

@jellemdekker
Copy link
Contributor

  1. Could you please explain what does read -rp enhances?

From the bash manpage about the built-in read command:

-p prompt: output the string PROMPT without a trailing newline before
attempting to read
-r: do not allow backslashes to escape any characters

  1. What about || return?

From shellcheck's wiki:

Rationale:
cd can fail for a variety of reasons: misspelled paths, missing directories, missing permissions, broken symlinks and more.

If/when it does, the script will keep going and do all its operations in the wrong directory. This can be messy, especially if the operations involve creating or deleting a lot of files.

To avoid this, make sure you handle the cases when cd fails. Ways to do this include

cd foo || exit as suggested to just abort immediately
if cd foo; then echo "Ok"; else echo "Fail"; fi for custom handling
<(cd foo && cmd) as an alternative to <(cd foo || exit; cmd) in <(..), $(..) or ( )

Except instead of cd foo || exit, it's now cd foo || return.

@jellemdekker
Copy link
Contributor

@sayem314 Please split up your pull request so that the code for support for custom DNS is in a separate PR.

@sayem314
Copy link
Contributor Author

I’m on vacation for Ramadan. Can’t really make pull request now. I have to update it after Eid. In the meantime if anyone would like to work on that I have no problem with that 🙂 And sorry about real late reply 😔

@sayem314 sayem314 mentioned this pull request Jun 21, 2018
@sayem314
Copy link
Contributor Author

sayem314 commented Jul 6, 2018

Closing this in favor of new PR

@angristan
Copy link
Owner

But custom DNS is gone D:

@sayem314
Copy link
Contributor Author

Oh yeah. Forgot about that. Also I broke my wrist 😔

Anyway I will make sure to pull request for custom DNS shortly.

@sayem314 sayem314 mentioned this pull request Jul 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants