Skip to content

Fix parsing of hints containing newlines and square brackets #196

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

Merged
merged 7 commits into from
Sep 3, 2022
Merged

Conversation

BelKed
Copy link
Contributor

@BelKed BelKed commented Aug 26, 2022

A long time ago, I noticed that hints with newlines are parsed incorrectly – the newline is ignored, and there's not even whitespace instead.
Also, part of the hint in square brackets shouldn't be changed, as it's already decoded.


Sorry for the use of RegEx. When I saw, how complexly it's implemented on the Geocaching website, I just couldn't refuse to use it...

Copy link
Owner

@tomasbedrich tomasbedrich left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the change!

@FriedrichFroebel
Copy link
Collaborator

FriedrichFroebel commented Aug 28, 2022

Looks okay for me, but I have been thinking about this topic in some more depth (considering the experience from the cache listings I observed in the last years) and I am not sure if we should improve this a little bit further.

While storing the "migrated" hint as cache.hint makes sense, we might want to provide a way to access the original hint text (in "encrypted" format). cache.hint might be derived from the original text during access instead of during parsing (we can still perform some similar caching as for the lazy loading, as functools.cached_property would require us to require Python 3.8). Background is that sometimes cache owners decide to manually "encrypt" the hint to wrap it into square brackets, to make it harder for the cacher to access it. This would include a way to enable/disable the bracket handling of the ROT13 implementation as well. Giving access to the original hint would allow the library user to handle the different cases himself. What do you think about this approach?

Additionally, you just added network-based tests for the actual parsing. Nevertheless, I probably would consider to add low-level tests for the utility methods as well, which do not depend on real data. This would ensure that certain edge cases (like unmatched brackets or similar) are handled correctly.

@FriedrichFroebel FriedrichFroebel self-requested a review August 28, 2022 13:38
@FriedrichFroebel
Copy link
Collaborator

Due to merging your other PR, there are some conflicting files now, which require manual merges to rebase the code on the current master.

@BelKed
Copy link
Contributor Author

BelKed commented Sep 2, 2022

Looks okay for me, but I have been thinking about this topic in some more depth (considering the experience from the cache listings I observed in the last years) and I am not sure if we should improve this a little bit further.

While storing the "migrated" hint as cache.hint makes sense, we might want to provide a way to access the original hint text (in "encrypted" format). cache.hint might be derived from the original text during access instead of during parsing (we can still perform some similar caching as for the lazy loading, as functools.cached_property would require us to require Python 3.8). Background is that sometimes cache owners decide to manually "encrypt" the hint to wrap it into square brackets, to make it harder for the cacher to access it. This would include a way to enable/disable the bracket handling of the ROT13 implementation as well. Giving access to the original hint would allow the library user to handle the different cases himself. What do you think about this approach?

I think it's a good idea, but I'm not planning a change handling of hints in this PR. As the title says, it just fixes parsing. So, feel free to open an issue referencing that :)

@BelKed
Copy link
Contributor Author

BelKed commented Sep 2, 2022

Additionally, you just added network-based tests for the actual parsing. Nevertheless, I probably would consider to add low-level tests for the utility methods as well, which do not depend on real data. This would ensure that certain edge cases (like unmatched brackets or similar) are handled correctly.

I added some tests for edge cases in 7b74c55.
Those values are also confirmed with the output of the convertROTStringWithBrackets() function on Geocaching website.

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. Looks good to me now.

@FriedrichFroebel
Copy link
Collaborator

I think it's a good idea, but I'm not planning a change handling of hints in this PR. As the title says, it just fixes parsing. So, feel free to open an issue referencing that :)

No big deal. Nobody has actually requested this so far (apart from my comment above, although this just has been a rough idea), so we probably should be safe to just ignore it for now. We can always decide to change this in a future version.

@FriedrichFroebel FriedrichFroebel merged commit ac0ed30 into tomasbedrich:master Sep 3, 2022
@BelKed BelKed deleted the fix-hint branch September 3, 2022 15:52
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.

3 participants