[openstreetmap/openstreetmap-website] Allow location to be searched with exiftool format #4040 (PR #4041)

[Andy Allan]

@shdpl Thank you for your pull request.

I'm not a fan of making a super-complex regexp even more complicated, to support what I suspect might be quite a niche activity for possibly very small number of mappers!

In any case, this fails the rubocop test for line length (and the current limit of 248 is only a "TODO", not a reasonable length in itself) which is a good indication that a better solution is required.

My main concern is to make this code more maintainable, and that means more understandable. I had to investigate what `(?:` means (non-capturing group) and I certainly would struggle to explain what the whole regexp does if someone else asks me to explain it. So when someone (inevitably) comes along in the future and needs a change to or bugfix for that regexp, is it going to be easier or harder for them to do so? While only a small change, I think this makes it even harder.

I'd prefer to see that part of the code refactored, for improved readability. For example, I like the approach used by logstash with its grok patterns, which uses interpolation to build up long complex regexps from component parts (see [example](https://github.com/logstash-plugins/logstash-patterns-core/blob/main/patterns/legacy/grok-patterns)). It's possible to use interpolation within regexps in ruby too. This would allow creating some basic parts of the regexps (e.g. the commonly used `\d{1,3}(\.\d*)?` pattern), giving them clear names, and building the final regexps out of multiple named components. That would make it much easier to understand and add more supported formats.

I also suspect that refactoring that parsing code out of the controller and into a library might make it easier to test and maintain.

Overall, however, without a more compelling use-case I personally don't think any of this is worth the effort. I can see how loading images into an editor (e.g. iD, JOSM) can be super useful but I'm not sure how often mappers will be using exiftool and pasting the results into the search box.

So I'll leave it to you. This PR can't be merged as-is, due to the line length. If you want to add another format, then the code needs to be refactored first, probably in a separate PR, to be much easier to read, understand, and maintain (and easier to review PRs too! :smile: )

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/4041#issuecomment-1591117792
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/4041/c1591117792 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20230614/97f68dd5/attachment.htm>