[openstreetmap/openstreetmap-website] Add URL prettification for plain texts (PR #6518)

[Pablo Brasero]

@pablobm commented on this pull request.

Why, doesn't this read so much better! 🎉 

The good thing is that I can review the code more thoroughly.

The bad thing is that I find new details to comment on 😝 

> @@ -139,6 +139,35 @@ fossgis_valhalla_url: "https://valhalla1.openstreetmap.de/route"
 # Endpoints for Wikimedia integration
 wikidata_api_url: "https://www.wikidata.org/w/api.php"
 wikimedia_commons_url: "https://commons.wikimedia.org/wiki/"
+linkify:
+  detection_rules:
+    - patterns: ["node/(?<id>\\d+)", "node (?<id>\\d{5,})", "n ?(?<id>\\d{5,})"]

I think these would be more readable if formatted as multiline YAML arrays:

```suggestion
    - patterns:
      - "node/(?<id>\\d+)"
      - "node (?<id>\\d{5,})"
      - "n ?(?<id>\\d{5,})"
```

>      end
 
     private
 
+    def expand_link_shorthands(text)
+      Array.wrap(Settings.linkify&.detection_rules)
+           .select { |rule| rule.path_template && rule.patterns.is_a?(Array) }
+           .flat_map { |rule| linkify_detection_rule_to_gsub(rule) }
+           .reduce(text) { |text, (pattern, replacement)| text.gsub(pattern, replacement) }
+    end
+
+    def linkify_detection_rule_to_gsub(rule)

Right, so this turns one of `linkify.detection_rules` into a `(pattern, replacement)` pair. 

I had no idea of what the method name meant and had to read the whole thing 😄 I was reading it as "linkify X to Y", with "linkify" as a verb and "to" linking it to a result. Instead it should be read as "X to Y", with "linkify" as part of a noun that gets turned into a different noun.

Can we come up with a better name? How about: `parse_linkify_detection_rule_into_gsub_pair`? I think that clarifies the verb and the result. Open to other options!

> +      Array.wrap(Settings.linkify&.detection_rules)
+           .select { |rule| rule.path_template && rule.patterns.is_a?(Array) }
+           .flat_map { |rule| linkify_detection_rule_to_gsub(rule) }

Interestingly, this whole thing does not depend on the argument `text`. I would separate it into a different method. Something like this:


```ruby
    def self.gsub_patterns_for_linkify
      Array
        .wrap(Settings.linkify&.detection_rules)
        .select { |rule| rule.path_template && rule.patterns.is_a?(Array) }
        .flat_map { |rule| linkify_detection_rule_to_gsub(rule) }
     end
```

Perhaps should be memoised? It does feel like waste, having to do all this work every time. But memoisation can bring its own issues.

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

Message ID: <openstreetmap/openstreetmap-website/pull/6518/review/3540190412 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20251204/9a5aac90/attachment.htm>