@pablobm commented on this pull request.
I wonder if the Markdown versions should still linkify, just not within code
blocks. More difficult to implement though, so not for this PR.
> @@ -88,25 +88,19 @@ def sanitize(text)
end
def linkify(text, mode = :urls)
- ERB::Util.html_escape(text)
- .then { |html| expand_link_shorthands(html) }
- .then { |html| expand_host_shorthands(html) }
- .then { |html| auto_link(html, mode) }
- .html_safe
+ link_attr = 'rel="nofollow noopener noreferrer" dir="auto"'
+ ERB::Util
+ .html_escape(text)
Sane indentation! Yes, please, more of this 😄
> @@ -88,25 +88,19 @@ def sanitize(text)
end
def linkify(text, mode = :urls)
- ERB::Util.html_escape(text)
- .then { |html| expand_link_shorthands(html) }
- .then { |html| expand_host_shorthands(html) }
- .then { |html| auto_link(html, mode) }
- .html_safe
+ link_attr = 'rel="nofollow noopener noreferrer" dir="auto"'
+ ERB::Util
+ .html_escape(text)
+ .then { |html| expand_link_shorthands(html) }
+ .then { |html| expand_host_shorthands(html) }
+ .then { |html| Rinku.auto_link(html, mode, link_attr) { |url|
shorten_link(shorten_hosts(url)) } }
Why did you decide to move this back out of the method to here?
> @@ -119,41 +113,40 @@ def expand_host_shorthands(text)
[Settings.linkify_hosts, Settings.linkify_hosts_replacement],
[Settings.linkify_wiki_hosts, Settings.linkify_wiki_hosts_replacement]
]
- .select { |hosts, replacement| replacement && hosts&.any? }
- .reduce(text) do |text, (hosts, replacement)|
-
text.gsub(/(?<=^|#{URL_UNSAFE_CHARS})\b#{Regexp.escape(replacement)}/) do
- "#{Settings.server_protocol}://#{hosts[0]}"
+ .map { |hosts, replacement| Struct.new(:hosts,
:host_replacement).new(hosts, replacement) }
I don't think that these structs buy us anything. They wrap data that then is
only used in the same method, with the same names. If this data was passed on
to other methods, then perhaps it would make sense, but I don't see it here.
Also, I did some cursory benchmarking and I'm seeing the struct versions
running at half the speed of the non-struct ones. However this is probably not
a big deal in context, and solvable by not creating the struct class
(`Struct.new`) every time, I think.
> - scheme_host, host, path = m.captures
- if hosts&.include?(host)
- path = yield(path) if block_given?
- if hosts_replacement
- "#{hosts_replacement}#{path}"
- else
- "#{scheme_host}#{path}"
- end
- end || url
- end || url
+ def shorten_hosts(url)
+ [
+ [Settings.linkify_hosts, Settings.linkify_hosts_replacement],
+ [Settings.linkify_wiki_hosts, Settings.linkify_wiki_hosts_replacement,
Settings.linkify_wiki_optional_path_prefix]
+ ]
+ .map { |hosts, replacement, prefix| Struct.new(:hosts,
:host_replacement, :optional_path_prefix).new(hosts, replacement, prefix) }
Same here about the structs.
> @@ -119,41 +113,40 @@ def expand_host_shorthands(text)
[Settings.linkify_hosts, Settings.linkify_hosts_replacement],
[Settings.linkify_wiki_hosts, Settings.linkify_wiki_hosts_replacement]
]
Not really about this PR, but I don't like how we do this processing chain for
a static array with only two elements. I think it would be more readable by
moving the chain to a method and then doing something like this:
```ruby
[
expand_shorthands(Settings.linkify_hosts, Settings.linkify_hosts_replacement),
expand_shorthands(Settings.linkify_wiki_hosts,
Settings.linkify_wiki_hosts_replacement)
]
```
> + [Settings.linkify_hosts, Settings.linkify_hosts_replacement],
+ [Settings.linkify_wiki_hosts, Settings.linkify_wiki_hosts_replacement,
Settings.linkify_wiki_optional_path_prefix]
+ ]
+ .map { |hosts, replacement, prefix| Struct.new(:hosts,
:host_replacement, :optional_path_prefix).new(hosts, replacement, prefix) }
+ .reduce(url) do |url, rule|
+ %r{^(https?://([^/]*))(.*)$}.match(url) do |m|
+ scheme_host, host, path = m.captures
+ if rule.hosts&.include?(host)
+ path = path.sub(Regexp.new(rule.optional_path_prefix || ""), "")
+ if rule.host_replacement
+ "#{rule.host_replacement}#{path}"
+ else
+ "#{scheme_host}#{path}"
+ end
+ end || url
+ end || url
And similarly to `auto_link`: why remove `shorten_host` and move the code here?
Chain methods with blocks that are in turn multi-level are difficult to follow.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/6674#pullrequestreview-3641374364
You are receiving this because you are subscribed to this thread.
Message ID:
<openstreetmap/openstreetmap-website/pull/6674/review/[email protected]>_______________________________________________
rails-dev mailing list
[email protected]
https://lists.openstreetmap.org/listinfo/rails-dev
