<p></p>
<p><b>@pablobm</b> commented on this pull request.</p>
<p dir="auto">Why, doesn't this read so much better! 🎉</p>
<p dir="auto">The good thing is that I can review the code more thoroughly.</p>
<p dir="auto">The bad thing is that I find new details to comment on 😝</p><hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/6518#discussion_r2589204262">config/settings.yml</a>:</p>
<pre style='color:#555'>> @@ -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,})"]
</pre>
<p dir="auto">I think these would be more readable if formatted as multiline YAML arrays:</p>
⬇️ Suggested change
<pre style="color: #555">- - patterns: ["node/(?<id>\\d+)", "node (?<id>\\d{5,})", "n ?(?<id>\\d{5,})"]
+ - patterns:
+ - "node/(?<id>\\d+)"
+ - "node (?<id>\\d{5,})"
+ - "n ?(?<id>\\d{5,})"
</pre>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/6518#discussion_r2589250958">lib/rich_text.rb</a>:</p>
<pre style='color:#555'>> 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)
</pre>
<p dir="auto">Right, so this turns one of <code class="notranslate">linkify.detection_rules</code> into a <code class="notranslate">(pattern, replacement)</code> pair.</p>
<p dir="auto">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.</p>
<p dir="auto">Can we come up with a better name? How about: <code class="notranslate">parse_linkify_detection_rule_into_gsub_pair</code>? I think that clarifies the verb and the result. Open to other options!</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/6518#discussion_r2589291906">lib/rich_text.rb</a>:</p>
<pre style='color:#555'>> + 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) }
</pre>
<p dir="auto">Interestingly, this whole thing does not depend on the argument <code class="notranslate">text</code>. I would separate it into a different method. Something like this:</p>
<div class="highlight highlight-source-ruby" dir="auto"><pre class="notranslate"> <span class="pl-k">def</span> <span class="pl-smi">self</span><span class="pl-kos">.</span><span class="pl-en">gsub_patterns_for_linkify</span>
<span class="pl-v">Array</span>
<span class="pl-kos">.</span><span class="pl-en">wrap</span><span class="pl-kos">(</span><span class="pl-v">Settings</span><span class="pl-kos">.</span><span class="pl-en">linkify</span>&.<span class="pl-en">detection_rules</span><span class="pl-kos">)</span>
<span class="pl-kos">.</span><span class="pl-en">select</span> <span class="pl-kos">{</span> |<span class="pl-s1">rule</span>| <span class="pl-s1">rule</span><span class="pl-kos">.</span><span class="pl-en">path_template</span> && <span class="pl-s1">rule</span><span class="pl-kos">.</span><span class="pl-en">patterns</span><span class="pl-kos">.</span><span class="pl-en">is_a?</span><span class="pl-kos">(</span><span class="pl-v">Array</span><span class="pl-kos">)</span> <span class="pl-kos">}</span>
<span class="pl-kos">.</span><span class="pl-en">flat_map</span> <span class="pl-kos">{</span> |<span class="pl-s1">rule</span>| <span class="pl-en">linkify_detection_rule_to_gsub</span><span class="pl-kos">(</span><span class="pl-s1">rule</span><span class="pl-kos">)</span> <span class="pl-kos">}</span>
<span class="pl-k">end</span></pre></div>
<p dir="auto">Perhaps should be memoised? It does feel like waste, having to do all this work every time. But memoisation can bring its own issues.</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/6518#pullrequestreview-3540190412">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLKRP57DJFUEQIAWJZ34ABAG7AVCNFSM6AAAAACLXBWDSCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTKNBQGE4TANBRGI">unsubscribe</a>.<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/AAK2OLKLOTQA5O6Z2Y7IRWL4ABAG7A5CNFSM6AAAAACLXBWDSCWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTWTAMCMY.gif" height="1" width="1" alt="" /><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message ID: <span><openstreetmap/openstreetmap-website/pull/6518/review/3540190412</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/openstreetmap/openstreetmap-website/pull/6518#pullrequestreview-3540190412",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/6518#pullrequestreview-3540190412",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>