@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/[email protected]>_______________________________________________
rails-dev mailing list
[email protected]
https://lists.openstreetmap.org/listinfo/rails-dev
