Updated RestCountryApi and RestCountryApiV2 BASE_URI #13

allansifuna · 2021-09-28T11:29:24Z

I have modified BASE_URI to https://restcountres.com
I have modified the tests

SteinRobert

Thank you very much for your PR. I do really appreciate it.
First of all, I do agree, that it makes sense to move to restcountries.com.

I noticed you made some changes, which are actually unrelated to the goal of the PR (changing the base url).

To keep the commit clean and readable I have requested some changes.

Bumping the version of the package in the PR is not a good idea. I will bump the package version as soon, as the PR is fine.
However, bumping it on patch level seems really wrong to me, since this is definitely a breaking change (another endpoint is used).

I believe there are 2 mistakes, when it comes to get_country_by_country_code. Please have a look at that, too.

Another thing. When you're done - could you please rebase the branch and squash the commits? This allows us to keep a clean and readable commit history.

Again, I do really appreciate the effort. So, thank you very much.
Looking forward to your changes.

SteinRobert · 2021-10-05T06:50:17Z

README.md

@@ -22,11 +22,13 @@ Just import the API Wrapper and start using it!::
 # v1 from restcountries import RestCountryApi as rapi
 from restcountries import RestCountryApiV2 as rapi

+


Could you please remove this linebreak?

SteinRobert · 2021-10-05T06:50:45Z

README.md

 def foo(name):
    country_list = rapi.get_countries_by_name('France')
-```


Why move this further done? Could you please change it back?

SteinRobert · 2021-10-05T06:52:54Z

README.md

 def foo(name):
-    country_list = rapi.get_countries_by_name("France" ,filters=["name","currencies","capital"])
+    country_list = rapi.get_countries_by_name(


Why change the formatting of the function call? The formatting does not correspond to the black formatting rules.

SteinRobert · 2021-10-05T06:53:34Z

restcountries/__init__.py

@@ -2,4 +2,4 @@
 from restcountries.base import RestCountryApi
 from restcountries.base import RestCountryApiV2

-__version__ = "1.0.0"
+__version__ = "1.0.1"


I would prefer to bump the version in a dedicated PR.

SteinRobert · 2021-10-05T06:54:07Z

restcountries/base.py

-# s = RestCountryApiV2()
-
-# s.get_countries_by_country_codes(["ke", "ug"],filters=["name","capital","currencies"])
+# TODO


Please remove the TODO from the code.

SteinRobert · 2021-10-05T06:55:04Z

restcountries/tests/v2/conftest.py

@@ -0,0 +1,295 @@
+import pytest


By changing the file's directory you make it really hard to compare the changes. Please change it back to the original directory structure.

SteinRobert · 2021-10-05T06:56:39Z

restcountries/tests/v2/conftest.py

+from restcountries.base import Country
+from restcountries.tests.countries_data import RSA, NGR, EGY, KEN
+
+base_url = "https://restcountries.com/rest/v2"


According to PEP8, which the module follows, constants should be uppercased.

SteinRobert · 2021-10-05T07:00:39Z

restcountries/tests/v2/conftest.py

+    Mock requests for getting a country by its country code and filters the response.
+    """
+    za_url = base_url + "/alpha/za?fields=name;capital"
+    requests_mock.get(za_url, json=[RSA])


This looks wrong. The endpoint does not return a list.

SteinRobert · 2021-10-05T07:01:32Z

restcountries/tests/v2/conftest.py

+    Mock requests for getting a country by its country code.
+    """
+    za_url = base_url + "/alpha/za"
+    requests_mock.get(za_url, json=[RSA])


This looks wrong. The endpoint does not actually return a list.

SteinRobert · 2021-10-05T07:03:06Z

.github/workflows/lint.yml

@@ -20,4 +20,5 @@ jobs:
          python -m pip install --upgrade pre-commit
          python -m pip install -e '.[d]'
      - name: Lint
-        run: pre-commit run --all-files --show-diff-on-failure
+        run: |
+          pre-commit run --all-files --show-diff-on-failure


Why add the linebreak here? This does not have anything to do with the actual topic of the PR.

Updated BASE_URI#12 Update Base URI #12 updated lint.yml Update Base URI #12 modifed readme.md Bug fixes Updated BASE_URI Updated BASE_URI

allansifuna · 2021-10-05T10:33:28Z

I have worked on the changes as requested. All is well now.

amatosg · 2021-10-06T23:54:15Z

restcountries/base.py

@@ -3,7 +3,7 @@


 class RestCountryApi:
-    BASE_URI = "https://restcountries.eu/rest/v1"
+    BASE_URI = "https://restcountries.com/v1"


just to let you know that V1 does not exists

@amatosg thanks for letting us now. I guess we could remove the support for v1 then.

SteinRobert · 2021-10-07T06:03:31Z

LGTM thank you. I'll open a dedicated issue to remove v1 support.

SteinRobert requested changes Oct 5, 2021

View reviewed changes

SteinRobert mentioned this pull request Oct 5, 2021

Restcountries.eu is no longer working #12

Closed

Updated BASE_URI

fbd95ff

Updated BASE_URI#12 Update Base URI #12 updated lint.yml Update Base URI #12 modifed readme.md Bug fixes Updated BASE_URI Updated BASE_URI

Updated BASE_URI

4294e8f

amatosg reviewed Oct 6, 2021

View reviewed changes

SteinRobert approved these changes Oct 7, 2021

View reviewed changes

SteinRobert merged commit df22994 into SteinRobert:master Oct 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated RestCountryApi and RestCountryApiV2 BASE_URI #13

Updated RestCountryApi and RestCountryApiV2 BASE_URI #13

allansifuna commented Sep 28, 2021

SteinRobert left a comment

SteinRobert Oct 5, 2021

SteinRobert Oct 5, 2021

SteinRobert Oct 5, 2021

SteinRobert Oct 5, 2021

SteinRobert Oct 5, 2021

SteinRobert Oct 5, 2021

SteinRobert Oct 5, 2021

SteinRobert Oct 5, 2021

SteinRobert Oct 5, 2021

SteinRobert Oct 5, 2021

allansifuna commented Oct 5, 2021

amatosg Oct 6, 2021

SteinRobert Oct 7, 2021

SteinRobert commented Oct 7, 2021

		@@ -22,11 +22,13 @@ Just import the API Wrapper and start using it!::
		# v1 from restcountries import RestCountryApi as rapi
		from restcountries import RestCountryApiV2 as rapi

Updated RestCountryApi and RestCountryApiV2 BASE_URI #13

Updated RestCountryApi and RestCountryApiV2 BASE_URI #13

Conversation

allansifuna commented Sep 28, 2021

SteinRobert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allansifuna commented Oct 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteinRobert commented Oct 7, 2021