pycaching icon indicating copy to clipboard operation
pycaching copied to clipboard

Get country and state from a cache

Open CachingFoX opened this issue 4 years ago • 4 comments

The PR extends class Cache to query the country and state of a cache.

The PR includes two new helper classes:

  • CountryState - Used to keep the pair of country and state Id
  • CountryStateDict - Used to map Groudspeak's country and state Ids to human-readable names and vice versa.

The PR also includes a set of classes to handle I18N behavior of pages. All languages from the website are supported, but not full implemented.

P.S. Sorry for the confusion, I delete the original branch in my fork and therefore the old PR was delete, too :-/

CachingFoX avatar Dec 03 '21 13:12 CachingFoX

PR is ready for review (and hopefully for merge)

CachingFoX avatar Dec 03 '21 14:12 CachingFoX

As this compromises a lot of modified/new lines, I will wait for @tomasbedrich to have a look at it.

Some remarks from my side:

  • Please rebase on the latest master to resolve the merge conflict.
  • Some of the docstrings are missing or incomplete (example: incomplete :return: lines). Could you try to complete them?
  • If I understand correctly, set_website_language will change the actual display language of the running user. As this should only be required for the tests, we might not want to make it part of the public API.
  • We probably want to add the new files to the docs.
  • While we still officially support Python 3.5, we cannot use the f-strings you use (see https://github.com/tomasbedrich/pycaching/blob/master/pyproject.toml#L29 as well).
  • The large dictionaries make the corresponding file rather lengthy and hard to read. Is there anything we can do to improve these?

FriedrichFroebel avatar Dec 05 '21 17:12 FriedrichFroebel

@CachingFoX We can discuss details later, but most importantly – I am sorry, but I don't get the point of this PR.

There are three main changes, judging by regexes introduced in I18Helpers, Cache + Geocaching API changes:

  1. Change cache author parsing – from my perspective, this shouldn't change – see my other comment.
  2. Add country parsing – What is the motivation for it? If I need to get a human readable location of a geocache, I can use reverse geocoding. If reverse geocoding isn't possible, what is the motivation of parsing the country in such detail?
  3. Add locale switch possibility – From my perspective, this is unwanted feature. Pycaching aims to abstract the language away. There may be places where we fail (eg. datetime formats are quite hard to guess), but I would rather put my effort into fixing such issue. Why do you think, it is needed?

Next time, if you want to avoid doing a big bulk of work before you get any feedback, consider creating an issue (according to suggested workflow) where we can discuss the idea beforehand.

tomasbedrich avatar Dec 06 '21 00:12 tomasbedrich

* Please rebase on the latest master to resolve the merge conflict.

Thank you for the reminder, I overlook this warning.

* Some of the docstrings are missing or incomplete (example: incomplete `:return:` lines). Could you try to complete them?

I will do.

* If I understand correctly, `set_website_language` will change the actual display language of the running user. As this should only be required for the tests, we might not want to make it part of the public API.

Yes it is required for testing I18N features and must not be part of the public API.

* We probably want to add the new files to the docs.

* While we still officially support Python 3.5, we cannot use the f-strings you use (see https://github.com/tomasbedrich/pycaching/blob/master/pyproject.toml#L29 as well).

Thank you for the hint. I had oriented myself to the CI builds that are available for pycaching for Python 3.6 to 3.10. But of course, I can change f-string to the classic one.

* The large dictionaries make the corresponding file rather lengthy and hard to read. Is there anything we can do to improve these?

I understand your comment. I don't know a good way to solve this issue. Lookup file? I don't know.

Before I will continue with rework, I will check the remarks of tomasbedrich.

CachingFoX avatar Dec 07 '21 07:12 CachingFoX