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

feat: validate the cc when using city params #175

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

Conversation

gandol
Copy link

@gandol gandol commented Feb 10, 2024

when just use lat lon the countty not correct, with these fix can check the valid county by the cc

speedtest/server.go Outdated Show resolved Hide resolved
@gandol
Copy link
Author

gandol commented Feb 11, 2024

previous code not realy filter the cc, i just update it

@r3inbowari
Copy link
Collaborator

r3inbowari commented Feb 23, 2024

Hi, @gandol. It doesn't seem to work because cc was not set when using lat and lon.

@gandol
Copy link
Author

gandol commented Feb 28, 2024

yeah its true, if the cc was not filtered set because of this
if s.config.Location != nil

so the filter can run only if the cc was set before
i think when use lat lon the expected result was not filtered by the cc

@r3inbowari
Copy link
Collaborator

We can preload cc from here.
const speedTestConfigUrl = "https://www.speedtest.net/speedtest-config.php"

@r3inbowari
Copy link
Collaborator

In fact, filtering them has no practical effect, because the lat and lon parameters are determined by the ookla's server and they are always correct. Can you list a use case?

@gandol
Copy link
Author

gandol commented Feb 29, 2024

try using yishun in CityFlag config
the result not only shown list of server from cc of SG but there is server from MY and ID to

@r3inbowari
Copy link
Collaborator

r3inbowari commented Feb 29, 2024

This is normal, because location is searched within a rough radius. Even if you use the official speedtest, you will still get these results.

The intention of Location is to modify your current location without doing too much processing on the results. It should not be a filter.

@r3inbowari
Copy link
Collaborator

r3inbowari commented Feb 29, 2024

For now, I think the best way is to add a --cc flag to filter them:

  1. disabled by default.
  2. auto attach by ookla's API. eg: --cc=auto
  3. filter based on the provided cc. eg: --cc=SG

@r3inbowari
Copy link
Collaborator

r3inbowari commented Feb 29, 2024

try using yishun in CityFlag config the result not only shown list of server from cc of SG but there is server from MY and ID to

CityFlag and CC(country code) are not related. CityFlag only maps to a build-in geographical coordinate.

@gandol
Copy link
Author

gandol commented Feb 29, 2024

For now, I think the best way is to add a --cc flag to filter them:

  1. disabled by default.
  2. auto attach by ookla's API. eg: --cc=auto
  3. filter based on the provided cc. eg: --cc=SG

if just use cc how to get the list of server, i was try use params of cc directly in url params can't get it
it must add cityflag or use keyword
or create an array of cc and keyword ?

@r3inbowari
Copy link
Collaborator

We can preload cc from here. const speedTestConfigUrl = "https://www.speedtest.net/speedtest-config.php"

  1. Through this api we can get the default cc, which depends on your network location.
  2. If the city flag is set, we can use the build-in cc instead of the default cc.

For now, I think the best way is to add a --cc flag to filter them:

  1. disabled by default.
  2. auto attach by ookla's API. eg: --cc=auto
  3. filter based on the provided cc. eg: --cc=SG

if just use cc how to get the list of server, i was try use params of cc directly in url params can't get it it must add cityflag or use keyword or create an array of cc and keyword ?

cc should be used with city location as much as possible.

@gandol
Copy link
Author

gandol commented Mar 2, 2024

@r3inbowari check my last commit, like that what you mean ?

Copy link
Collaborator

@r3inbowari r3inbowari left a comment

Choose a reason for hiding this comment

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

Thank you! I left some comments.

speedtest/server.go Outdated Show resolved Hide resolved
speedtest/server.go Outdated Show resolved Hide resolved
@r3inbowari
Copy link
Collaborator

r3inbowari commented Mar 2, 2024

I changed some code here.

examples:

./speedtest-go --city=tokyo -l // disable filter by default
./speedtest-go --city=tokyo --filter-cc=jp -l
./speedtest-go --city=hongkong --filter-cc=cn --filter-cc=hk -l  // excludes MO servers which close to HongKong and includes CN
./speedtest-go --city=hongkong --filter-cc=auto -l                  // auto attach which decided by your network provider
./speedtest-go --location=xx,xx --filter-cc=jp -l

If there is no server for this cc in the area you specify, the result will be empty.

Do you think this is ok?

@gandol
Copy link
Author

gandol commented Mar 5, 2024

I changed some code here.

examples:

./speedtest-go --city=tokyo -l // disable filter by default
./speedtest-go --city=tokyo --filter-cc=jp -l
./speedtest-go --city=hongkong --filter-cc=cn --filter-cc=hk -l  // excludes MO servers which close to HongKong and includes CN
./speedtest-go --city=hongkong --filter-cc=auto -l                  // auto attach which decided by your network provider
./speedtest-go --location=xx,xx --filter-cc=jp -l

If there is no server for this cc in the area you specify, the result will be empty.

Do you think this is ok?

hmm i think this is ok

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