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

Get country and state from a cache #170

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

Conversation

CachingFoX
Copy link
Contributor

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
Copy link
Contributor Author

PR is ready for review (and hopefully for merge)

@FriedrichFroebel
Copy link
Collaborator

FriedrichFroebel commented Dec 5, 2021

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?

@tomasbedrich
Copy link
Owner

tomasbedrich commented Dec 6, 2021

@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.

:param str language_code: language_TERRITORY
:return:
"""
self._request(self._urls["culture_set"], params={"model.SelectedCultureCode": language_code}, expect="raw")
Copy link
Owner

Choose a reason for hiding this comment

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

If I want to perform "Change the language of the geocaching.com website." in my browser, the call looks like this:

curl 'https://www.geocaching.com/api/proxy/web/v1/users/locale' -X PUT -H "...(headers-omitted)..." --data-raw '{"locale":"cs-CZ"}'

Can you explain what is play/culture/set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw this kind of call on the landing page of www.geocaching.com if you are not log in.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But see my answer to FriedrichFroebel about set_website_language() is is only necessary to the I18N based test.

@@ -743,7 +757,8 @@ def load(self):
raise errors.LoadError()
self.name = cache_details.find("h2").text

self.author = cache_details("a")[1].text
Copy link
Owner

Choose a reason for hiding this comment

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

I think this works correctly (for all languages) and shouldn't be removed.

Comment on lines -725 to -726
author = cache_details.find(id="ctl00_ContentBody_uxCacheBy").text
self.author = author[11:] # 11 = len("A cache by ")
Copy link
Owner

Choose a reason for hiding this comment

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

This is indeed an obsolete piece of code, replaced by self.author = cache_details("a")[1].text. Thus it can be deleted without replacement.

@CachingFoX
Copy link
Contributor Author

* 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.

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