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

Fix getting user logs #233

Merged
merged 2 commits into from
Dec 28, 2023
Merged

Conversation

Jakoma02
Copy link
Contributor

Due to changes on the Geocaching website, iterating my_logs() raises an exception due to a missing query argument "guid". This is caused by an upstream change at https://www.geocaching.com/my/logs.aspx, where the format of the URL that this library uses was changed to "https://www.geocaching.com/geocache/[WP]".

This PR modifies the my_logs() method to use WP to find the geocache details instead of guid, which fixes the problem.

@@ -494,8 +494,8 @@ def my_logs(self, log_type=None, limit=float("inf")):
break

link = row.find(class_="ImageLink")["href"]
guid = parse_qs(urlparse(link).query)["guid"][0]
current_cache = self._try_getting_cache_from_guid(guid)
wp = link.split("/")[4]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we make this more explicit somehow or check that the removed prefix is correct? For now, this seems to be "magic", as we do not have automated tests for this at the moment as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An explicit way of doing this might be using regular expressions, which are not recommended as per contribution guidelines. Also note that on line 470, a very similar extraction is done in a very similar way.

Would you consider this resolved if a comment is provided similarly to line 470? Or would you rather see explicit checking of the prefix or moving the "logic" to a separate function?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the easiest way, we can consider adding a comment with an example URL here to make this more clear.

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 added the comment in commit cdeeeca, please see if this is sufficient.

Copy link
Collaborator

@FriedrichFroebel FriedrichFroebel left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.

@FriedrichFroebel FriedrichFroebel merged commit 12e82a4 into tomasbedrich:master Dec 28, 2023
15 checks passed
@ohheyrj
Copy link

ohheyrj commented Jan 26, 2024

Has this been released yet or is it only in master at the moment?

@tomasbedrich
Copy link
Owner

Now it is released as 4.4.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants