Conversation
|
Please sync with master, the CI should now be fixed and the failing IE test marked as a known failure. |
|
Thank you for your review! I tried to address your comments. Let me know what you think :) |
rth
left a comment
rth
left a comment
There was a problem hiding this comment.
Thanks! A few last comments otherwise LGTM.
| self._data_path, self._data = self._get_data(country) | ||
| country_obj = _Country(country) | ||
| self.country = country_obj.name | ||
| self.download_path = country_obj.get_download_path() |
There was a problem hiding this comment.
Let's make this private as well,
| self.download_path = country_obj.get_download_path() | |
| self._download_path = country_obj.get_download_path() |
|
|
||
| @pytest.fixture(scope='session') | ||
| def country_gb_full(): | ||
| return pgeocode._Country('gb_full') |
There was a problem hiding this comment.
Typically session level fixtures are used for expensive operation (e.g. loading the data, or file access operations) once per session run.
To test _Country we should rather initialize it in each test that needs (and not use fixture).
|
|
||
|
|
||
| def test_that_get_clean_country_raises_value_error(): | ||
| with pytest.raises(ValueError): |
There was a problem hiding this comment.
| with pytest.raises(ValueError): | |
| with pytest.raises(ValueError, match="not a known country code"): |
|
Hi guys! Is it possible to merge this fix? It would still be great to have the possibility to use GB_full database instead GB |
4 participants
Countries.