[openstreetmap/openstreetmap-website] Move browse_tags_helper functionality to a library (PR #7304)

[Pablo Brasero]

@pablobm commented on this pull request.



> @@ -72,6 +72,30 @@ def link_follow(object)
     "nofollow" if object.tags.empty?
   end
 
+  def format_key(key)
+    TagLinker.format_key(key) do |hash|
+      title_options = hash.except(:text, :url, :type)
+      return hash[:text] unless hash[:url]
+
+      link_to(hash[:text], hash[:url], :title => t("browse.tag_details.#{hash[:type]}", *title_options))

`*` is for positional arguments while `**` is for keyword arguments. This is what you actually want:

```suggestion
      link_to(hash[:text], hash[:url], :title => t("browse.tag_details.#{hash[:type]}", **title_options))
```

When you use `*`, the method call becomes this:

```ruby
      link_to(hash[:text], hash[:url], :title => t("browse.tag_details.#{hash[:type]}", {:foo => "bar}))
```

And Ruby sees that as three positional arguments, instead of positional-keyword-positional.

> +      title_options = hash.except(:text, :url, :type)
+      return hash[:text] unless hash[:url]
+
+      link_to(hash[:text], hash[:url], :title => t("browse.tag_details.#{hash[:type]}", *title_options))
+    end
+  end
+
+  def format_value(key, value, html_only: false)
+    html = []
+    TagLinker.format_value(key, value) do |hash|
+      title_options = hash.except(:text, :url, :type, :html)
+      return html << hash[:html] if hash[:html] && html_only
+
+      return html << hash[:text] unless hash[:url]
+
+      html << link_to(hash[:text], hash[:url], :title => t("browse.tag_details.#{hash[:type]}", *title_options))

Same as above:

```suggestion
      html << link_to(hash[:text], hash[:url], :title => t("browse.tag_details.#{hash[:type]}", **title_options))
```

> @@ -72,6 +72,30 @@ def link_follow(object)
     "nofollow" if object.tags.empty?
   end
 
+  def format_key(key)
+    TagLinker.format_key(key) do |hash|
+      title_options = hash.except(:text, :url, :type)
+      return hash[:text] unless hash[:url]
+
+      link_to(hash[:text], hash[:url], :title => t("browse.tag_details.#{hash[:type]}", *title_options))
+    end
+  end
+
+  def format_value(key, value, html_only: false)
+    html = []
+    TagLinker.format_value(key, value) do |hash|
+      title_options = hash.except(:text, :url, :type, :html)
+      return html << hash[:html] if hash[:html] && html_only

You want a `next` here instead of a `return`. The return exists `BrowseHelper#format_value` completely. Not only it won't do the `safe_join`, but also it won't even allow `Taglinker.format_value` to do whatever it wants to do after calling the block internally.

This will put an end to the block only, allowing both methods to continue:

```suggestion
      next html << hash[:html] if hash[:html] && html_only
```

> +  def format_key(key)
+    TagLinker.format_key(key) do |hash|
+      title_options = hash.except(:text, :url, :type)
+      return hash[:text] unless hash[:url]
+
+      link_to(hash[:text], hash[:url], :title => t("browse.tag_details.#{hash[:type]}", *title_options))
+    end
+  end
+
+  def format_value(key, value, html_only: false)
+    html = []
+    TagLinker.format_value(key, value) do |hash|
+      title_options = hash.except(:text, :url, :type, :html)
+      return html << hash[:html] if hash[:html] && html_only
+
+      return html << hash[:text] unless hash[:url]

Same here:

```suggestion
      next html << hash[:text] unless hash[:url]
```

> +      html << ";"
+    end
+    html.pop if html.last == ";"
+    safe_join(html)

Easier to add the semicolons as part of the `safe_join`:

```suggestion
    end
    safe_join(html, ";")
```

>      elsif wdt = wikidata_links(key, value)
-      # IMPORTANT: Note that wikidata_links() returns an array of hashes, unlike for example wikipedia_link(),
-      # which just returns one such hash.
-      svg = button_tag :type => "button", :role => "button", :class => "btn btn-link float-end d-flex m-1 mt-0 me-n1 border-0 p-0 wdt-preview", :data => { :qids => wdt.pluck(:title) } do
+      yield({ :type => :html, :html => button_tag(:type => "button", :role => "button", :class => "btn btn-link float-end d-flex m-1 mt-0 me-n1 border-0 p-0 wdt-preview", :data => { :qids => wdt.pluck(:title) }) do

This is the problematic part: this uses Rails helpers (`button_tag`, `tag`, `concat`, `t`) in a context that doesn't have access to them. I don't have a quick solution for this; it can be done in a variety of ways.

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

Message ID: <openstreetmap/openstreetmap-website/pull/7304/review/5241264027 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20260917/64bdd3cd/attachment.htm>