search

tomasbedrich / pycaching

A Python 3 interface for working with Geocaching.com website.
https://pycaching.readthedocs.io/
GNU Lesser General Public License v3.0
61 stars 46 forks source link

Fix parsing of hints containing newlines and square brackets #196

Closed BelKed closed 2 years ago

BelKed commented 2 years ago

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

FriedrichFroebel commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/tomasbedrich/pycaching/pull/196/commits/7b74c5541be60f7d92579bf1168ded3ee78a3e1f. Those values are also confirmed with the output of the convertROTStringWithBrackets() function on Geocaching website.

FriedrichFroebel commented 2 years ago

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.