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

used shellcheck, some fixes, added new countries, added new support for url query parameter #10

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

YoshiMan
Copy link

I used shellcheck to validate the script.
see the commits

added new countries

https://github.com/koalaman/shellcheck
fixed SC2128: Expanding an array without an index only gives the first element.
https://github.com/koalaman/shellcheck/wiki/SC2128
https://github.com/koalaman/shellcheck
fixed SC2116: Useless echo? Instead of 'cmd $(echo foo)', just use 'cmd foo'
https://github.com/koalaman/shellcheck/wiki/SC2116

query, code and country variable to an array

changed case statement
fixed SC2162: read without -r will mangle backslashes.
https://github.com/koalaman/shellcheck/wiki/SC2162
SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
https://github.com/koalaman/shellcheck/wiki/SC2181
added support for 'ip_version' parameter
added support for 'use_mirror_status' parameter
@YoshiMan YoshiMan changed the title used shellcheck, some fixes, added new countries used shellcheck, some fixes, added new countries, added new support for url query parameter Nov 27, 2017
@deadhead420
Copy link
Owner

Looks good! Only thing is running default with no options does not work. I'll have to look at it more soon

@deadhead420
Copy link
Owner

Can you verify this? When running fetchmirrors without any options it shows usage info and not country select list.

@deadhead420
Copy link
Owner

The search function is broken

@deadhead420
Copy link
Owner

I will be more than happy to merge this and update on the AUR but the search function needs to be working. One of the problems is the line with: [ -z "$1" ] causes the program not to even use the search section, this line needs to be removed and only the line checking for root left. Beyond that when you enter the search function it fails when you select a country number.

@YoshiMan
Copy link
Author

When I invoke fetchmirrors without sudo and without any arguments it shows the (modified) usage info.
Or did I get you wrong?
my output from the "your" fetchmirror:


 Usage: fetchmirrors <opts> <args> Ex: fetchmirrors -q -s 4 -c US

 Options:
   -c --country
   Specify your country code(s): fetchmirrors -c US AU
   -d --nocolor
   Disable color prompts
   -h --help
   Display this help message
   -l --list
   Display list of country codes
   -q --noconfirm
   Disable confirmation messages
   -s --servers
   Number of servers to rank (default  6)
   -v --verbose
   Verbose output

 Use fetchmirrors without any option to prompt for country code(s)

my output with "my" fetchmirrors:


 Usage: fetchmirrors.sh <opts> <args> Ex: fetchmirrors.sh -q -s 4 -c US

 Options:
   -c --country
   Specify your country code(s): fetchmirrors.sh -c US AU
   -d --nocolor
   Disable color prompts
   -h --help
   Display this help message
   -l --list
   Display list of country codes
   -q --noconfirm
   Disable confirmation messages
   -s --servers
   Number of servers to rank (default  6)
   -v --verbose
   Verbose output
   -p --protocol
   the protocol to use either http or https, default is empty (both)
   -i --ip-version
   the ip version to use either 4 or 6, default is empty (both)
   -u --use-mirror-status
   use mirror status

 Use fetchmirrors.sh without any option to prompt for country code(s)

@YoshiMan
Copy link
Author

I dont understand what you mean by "The search function is broken", could you provide an sample search term or any further information. ;)

I have recorded my terminal via https://asciinema.org.

the parameterless call. https://asciinema.org/a/5ugAH0aTnlKZdpavY65Z4hdvi
the search function. https://asciinema.org/a/YWaPSpCbBhaqsHdg8yhHDjQwZ

Maybe you could do this to, so i can understand what you mean. :)

@YoshiMan
Copy link
Author

YoshiMan commented Dec 5, 2017

@deadhead420 could you provide some Information?
thank you.

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.

2 participants