[openstreetmap/openstreetmap-website] Add Communities page (#3301)

[Tom Hughes]

@tomhughes commented on this pull request.

This is just a few high level thoughts - the view stuff looks reasonably sane but while I haven't tried to understand the model stuff in detail yet it all looks pretty odd to me.

> @@ -0,0 +1,59 @@
+class Communities
+  require "yaml"
+
+  @local_chapters = {}
+
+  def self.local_chapters(locale)
+    @local_chapters[locale] = local_chapter_for(locale)

If that is meant to be memoising the value then you want `||=` not just `=`.

> @@ -0,0 +1,59 @@
+class Communities
+  require "yaml"
+
+  @local_chapters = {}
+
+  def self.local_chapters(locale)
+    @local_chapters[locale] = local_chapter_for(locale)
+  end
+
+  class << self

Why was `self.` good enough before but now you want to resort to black magic to define these class methods?

> @@ -0,0 +1,59 @@
+class Communities
+  require "yaml"
+
+  @local_chapters = {}
+
+  def self.local_chapters(locale)
+    @local_chapters[locale] = local_chapter_for(locale)
+  end
+
+  class << self
+    protected

Is this the reason for the black magic? Because you can't have a protected class method so you'd making them instance methods on the metaclass instead? Why do you want protected anyway - is something going to inherit from this?

> @@ -0,0 +1,59 @@
+class Communities
+  require "yaml"
+
+  @local_chapters = {}
+
+  def self.local_chapters(locale)
+    @local_chapters[locale] = local_chapter_for(locale)
+  end
+
+  class << self
+    protected
+
+    def local_chapter_for(locale)
+      @local_chapters_index = load_local_chapters

See previous comment about memoisation.

> @@ -106,6 +106,11 @@ def about
     @locale = params[:about_locale] || I18n.locale
   end
 
+  def communities
+    @locale = params[:communities_locale] || I18n.locale

Is language the correct way to be selecting a community? Shouldn't it be location driven?

> @@ -0,0 +1,59 @@
+class Communities

Why is this a model? It doesn't seem to act like one... As far as I can see you never actually even create an object of this class... Rather the class methods that look like they should be constructors for the model are actually just returning plain hashes?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/3301#pullrequestreview-737702168
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20210824/a0d817a9/attachment-0001.htm>