-
Notifications
You must be signed in to change notification settings - Fork 46
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
base: master
Are you sure you want to change the base?
Conversation
PR is ready for review (and hopefully for merge) |
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:
|
@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:
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") |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
author = cache_details.find(id="ctl00_ContentBody_uxCacheBy").text | ||
self.author = author[11:] # 11 = len("A cache by ") |
There was a problem hiding this comment.
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.
Thank you for the reminder, I overlook this warning.
I will do.
Yes it is required for testing I18N features and must not be part of the public API.
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.
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. |
The PR extends class Cache to query the country and state of a cache.
The PR includes two new helper classes:
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 :-/