<p></p>
<p><b>@gravitystorm</b> commented on this pull request.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2921#discussion_r521439457">app/helpers/title_helper.rb</a>:</p>
<pre style='color:#555'>> @@ -6,7 +6,7 @@ def self.coder
   def set_title(title = nil)
     if title
       @title = TitleHelper.coder.decode(title.gsub("<bdi>", "\u202a").gsub("</bdi>", "\u202c"))
-      response.headers["X-Page-Title"] = ERB::Util.u(@title + " | " + t("layouts.project_name.title"))
+      response.headers["X-Page-Title"] = ERB::Util.u("#{@title} | #{t('layouts.project_name.title')}")
</pre>
<blockquote>
<p>my natural assumption would be that concatenation would generally be higher performance</p>
</blockquote>
<p>I think concatenation is generally slower, since for <code>string1 + string2 + string3</code> you end up needing even more strings, since each <code>+</code> allocates a new string to hold the result of adding the two together, but...</p>
<blockquote>
<p>not that it matters most of the time</p>
</blockquote>
<p>Indeed! <g-emoji class="g-emoji" alias="smile" fallback-src="https://github.githubassets.com/images/icons/emoji/unicode/1f604.png">😄</g-emoji> So I'm much more interested in the consistency, and developer experience, unless there's some catastrophic performance difference.</p>
<blockquote>
<p>it might all become a bit unreadable.</p>
</blockquote>
<p>Yeah, it's certainly not straightforward to read. My local editor has different syntax highlighting than on github, and it's definitely easier on the eye. So overall, I'm still happy with the PR, but also happy to come up with an alternative approach.</p>
<p>I thought I'd try out the format() approach that you suggested, e.g.</p>
<div class="highlight highlight-source-diff"><pre><span class="pl-md"><span class="pl-md">-</span> response.headers["X-Page-Title"] = ERB::Util.u("#{@title} | #{t('layouts.project_name.title')}")</span>
<span class="pl-mi1"><span class="pl-mi1">+</span> response.headers["X-Page-Title"] = ERB::Util.u(format("%s | %s", @title, t('layouts.project_name.title')))</span></pre></div>
<p>... but that triggers another rubocop warning about labelling the string tokens in the format string. And with labelled tokens, we need to supply a hash of values. So in the end we get something like this:</p>
<div class="highlight highlight-source-diff"><pre><span class="pl-md"><span class="pl-md">-</span> response.headers["X-Page-Title"] = ERB::Util.u("#{@title} | #{t('layouts.project_name.title')}")</span>
<span class="pl-mi1"><span class="pl-mi1">+</span> response.headers["X-Page-Title"] = ERB::Util.u(format("%<title>s | %<project_title>s", :title => @title, :project_title => t("layouts.project_name.title")))</span></pre></div>
<p>I find this even harder to read, since the format string <code>"%<title>s | %<project_title>s"</code> looks sort of but not quite like an interpolated string e.g. <code>"#{title} | #{project_title}"</code>, and the line ends up being quite long too.</p>
<p>However, one final option is then to use an interpolated string, but to avoid putting complex stuff inside it, by unrolling it slightly e.g.</p>
<div class="highlight highlight-source-diff"><pre><span class="pl-md"><span class="pl-md">-</span> response.headers["X-Page-Title"] = ERB::Util.u("#{@title} | #{t('layouts.project_name.title')}")</span>
<span class="pl-mi1"><span class="pl-mi1">+</span> project_title = t 'layouts.project_name.title'</span>
<span class="pl-mi1"><span class="pl-mi1">+</span> response.headers["X-Page-Title"] = ERB::Util.u("#{@title} | #{project_title}")</span></pre></div>
<p>What do you think?</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2921#discussion_r521439457">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLI3YANV5BJT3C4BEJLSPKU4TANCNFSM4TCRTX2Q">unsubscribe</a>.<img src="https://github.com/notifications/beacon/AAK2OLLWBXT672RXSZ5PVDDSPKU4TA5CNFSM4TCRTX22YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOD56OINY.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/openstreetmap/openstreetmap-website/pull/2921#discussion_r521439457",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/2921#discussion_r521439457",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>