The implementation of render_social_share_buttons can be refactored in a number
of ways:
* `html_safe` **must not be used**. 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.
`opengraph_tags` helper for the use of `safe_join` to combine tags in loop.
* No need for a `render_` prefix in helper methods. See e.g. `opengraph_tags`
helper
* There are only two `opts` ever used in the method, `:title` and `:url`. 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.
* The `filter_allowed_sites` 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...
* The `filter_allowed_sites` 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.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/issues/5415
You are receiving this because you are subscribed to this thread.
Message ID: <openstreetmap/openstreetmap-website/issues/5...@github.com>
_______________________________________________
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
