@tomhughes commented on this pull request.
> + if key =~
> /^(#{SECONDARY_WIKI_PREFIX_PATTERN})?wikipedia(:(#{WIKIPEDIA_PROJECT_IDENTIFIER_PATTERN}))?$/o
+ lang = Regexp.last_match(3)
Why are we capturing things we're not going to use? How about making this:
```suggestion
if key =~
/^(?:#{SECONDARY_WIKI_PREFIX_PATTERN})?wikipedia(:(?:#{WIKIPEDIA_PROJECT_IDENTIFIER_PATTERN}))?$/o
lang = Regexp.last_match(1)
```
> page_lang = Regexp.last_match(1)
title_section = Regexp.last_match(2)
else
page_lang = lang
+ return nil unless page_lang
Doesn't this mean we now bail on a simple `wikipedia=` tag? It use to be that
the key match would default `lang` to `en` in that case but it no longer does
so it will be nil here causing us to bail?
> @@ -39,8 +39,8 @@ def test_format_value
html = format_value("phone", "+1 (234) 567-890 ; +22334455")
assert_dom_equal "<a href=\"tel:+1(234)567-890\" title=\"Call +1 (234)
567-890\">+1 (234) 567-890</a>; <a href=\"tel:+22334455\" title=\"Call
+22334455\">+22334455</a>", html
- html = format_value("wikipedia", "Test")
- assert_dom_equal "<a title=\"The Test article on Wikipedia\"
href=\"https://en.wikipedia.org/wiki/Test?uselang=en\";>Test</a>", html
+ html = format_value("wikipedia", "en:Test")
This would presumably have the issue where no language is given in the key or
the value except you changed it to provide a language in the value...
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/7136#pullrequestreview-4564589670
You are receiving this because you are subscribed to this thread.
Message ID:
<openstreetmap/openstreetmap-website/pull/7136/review/[email protected]>_______________________________________________
rails-dev mailing list
[email protected]
https://lists.openstreetmap.org/listinfo/rails-dev
