-
Notifications
You must be signed in to change notification settings - Fork 45
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
Conversation
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.
LGTM, thanks for the change!
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 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. |
Due to merging your other PR, there are some conflicting files now, which require manual merges to rebase the code on the current master. |
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 :) |
I added some tests for edge cases in 7b74c55. |
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.
Thanks. Looks good to me now.
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. |
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...