<p></p>
<p dir="auto">The implementation of render_social_share_buttons can be refactored in a number of ways:</p>
<ul dir="auto">
<li><code class="notranslate">html_safe</code> <strong>must not be used</strong>. It does not convert strings to a safe representation, it is an assertion that the string is always safe given every possible input. This method is probably too complex for us to assert this today, and certainly not to ensure that everything involved is perfectly safe when features are added or removed from the helper in future. See e.g. <code class="notranslate">opengraph_tags</code> helper for the use of <code class="notranslate">safe_join</code> to combine tags in loop.</li>
<li>No need for a <code class="notranslate">render_</code> prefix in helper methods. See e.g. <code class="notranslate">opengraph_tags</code> helper</li>
<li>There are only two <code class="notranslate">opts</code> ever used in the method, <code class="notranslate">:title</code> and <code class="notranslate">:url</code>. Since there are only two, it would be better to call the method with these explicitly, ideally as keyword arguments so that the order doesn't matter.</li>
<li>The <code class="notranslate">filter_allowed_sites</code> method behaves surprisingly. If you pass it some sites, it will only return a subset of those sites. However, if you pass an empty set, instead of an empty list you instead get a full list of sites that were not requested. However, it's probably unlikely to be important, because...</li>
<li>The <code class="notranslate">filter_allowed_sites</code> method is unnecessary and unused code. It should be removed, since unnecessary and used code is both a source of bugs, and also makes it harder for new developers in future, who need to read and understand it. Personally I've spent time trying to review how it works and what it's trying to do, before realising that it's not used anywhere.</li>
</ul>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/issues/5415">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLKQFA7D6E64G5BA7O32GGJELAVCNFSM6AAAAABT27MAIKVHI2DSMVQWIX3LMV43ASLTON2WKOZSG42DQMJUGE3TCMI">unsubscribe</a>.<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/AAK2OLNBJWOCER54GVFRTJL2GGJELA5CNFSM6AAAAABT27MAIKWGG33NNVSW45C7OR4XAZNFJFZXG5LFVJRW63LNMVXHIX3JMTHKHTKQR4.gif" height="1" width="1" alt="" /><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message ID: <span><openstreetmap/openstreetmap-website/issues/5415</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/openstreetmap/openstreetmap-website/issues/5415",
"url": "https://github.com/openstreetmap/openstreetmap-website/issues/5415",
"name": "View Issue"
},
"description": "View this Issue on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>