Getting a list of pages (for sitemaps, etc.) #193

jeremysmithco · 2015-04-03T21:57:32Z

Here's my take on getting a list of High Voltage pages for building sitemaps, based on this issue: #123

If you're using the sitemap_generator gem, you could do something like this in your config:

HighVoltage.page_ids.each do |page|
  add page, changefreq: 'monthly'
end

houndci-bot · 2015-04-03T21:57:36Z

lib/high_voltage.rb

@@ -3,6 +3,7 @@

 require 'high_voltage/configuration'
 require 'high_voltage/constraints/root_route'
+require 'high_voltage/page_collector'


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

jeremysmithco · 2015-04-07T22:01:15Z

I fixed the empty line, new line and "line too long" issues from Hound.

Should I make the double-quoted string changes? Seemed like most of the repo is using single quotes.

dgalarza · 2015-04-10T13:54:09Z

lib/high_voltage/configuration.rb

@@ -14,7 +14,8 @@ module Configuration
      :routes,
    )

-    attr_reader :action_caching, :action_caching_layout, :page_caching
+    attr_reader :action_caching, :action_caching_layout, :page_caching,
+                :page_ids


Do we need this attr_reader if we are defining page_ids on line 43?

Ah! You're right, not sure what I was thinking there.

dgalarza · 2015-04-16T22:17:38Z

Thanks for the pr @bentoncreation.

Should I make the double-quoted string changes? Seemed like most of the repo is using single quotes

We're slowly in the process of moving towards double quotes. Instead of doing this via one big sweep we're just making changes as we go through. As such, any places that are touched we recommend moving to double quote usage.

Some other thoughts:

What do you think about extracting another class to represent a page (or view)? That way instead of having a few private methods in the PageCollector class that need the same argument passed around, it could be contained within this new object.

Something along the lines of:

if page.valid?
  page_paths.push page.path
end

jeremysmithco · 2015-04-17T19:49:17Z

Sure thing @dgalarza ! Thanks for the review.

Okay, I'll follow Hound on double quotes, etc. (Although, I think it's wrong about 'detect' over 'find'. It seems to be thinking it's Enumerable, not the Find module.)

A Page class makes sense. How would that relate to the PageFinder class? Are they overlapping at all? I want to make sure I get the right abstraction.

dgalarza · 2015-04-17T19:54:48Z

I don't think that the new class would overlap with the PageFinder class since PageFinder is specifically responsible for finding a given static page based off an ID.

jeremysmithco · 2015-04-17T19:59:59Z

Ok, cool. Also, I noticed in your sample code, you changed page_ids to page_paths. The original issue suggested page_ids as the interface...would you rather go with page_paths?

dgalarza · 2015-04-17T20:02:54Z

Ah good question. I named it page_paths in my examples since it looked like you were collecting the path to a page with url_path. It feels like a better name imo however I don't feel to strongly about it if you feel otherwise.

jeremysmithco · 2015-04-17T20:20:43Z

Does the following make sense to you?

It seems like we're trying to make a distinction between file paths and URL paths. They could be different, since you can override the PageFinder with the Rot13PageFinder. To keep that distinction, we're calling the URL path a page_id.

PageFinder says: give me a page_id and I'll find the file path.

PageCollector is saying: give me a base file path and I'll return the page_ids for all the files I find there.

dgalarza · 2015-04-17T20:22:18Z

Makes sense to me, 👍

jeremysmithco · 2015-04-17T20:24:19Z

Great, thanks!

houndci-bot · 2015-05-20T18:55:47Z

lib/high_voltage/page_collector.rb

+    def page_ids
+      page_ids = []
+
+      Find.find(content_path) do |file_path|


Prefer detect over find.

This method is getting a bit long and seems to be responsible for quite a bit. What do you think about extracting a private method for collecting Page objects and then using a select on that collection for valid ones.

Something like:

def page_ids pages.select(&:valid?).map(&:id) end private def pages Find.find(content_path).map { |file_path| HighVoltage::Page.new(content_path, file_path) } end

Ah yes, much nicer!

jeremysmithco · 2015-05-20T19:43:00Z

Hey @dgalarza, I've broken out the Page class we talked about and I think I have things ready for you to review. Let me know how this looks when you get the chance. Thanks!

dgalarza · 2015-05-22T14:34:00Z

spec/high_voltage/page_spec.rb

+  end
+
+  it "is valid for a page" do
+    expect(page(full_file_path("exists.html.erb")).valid?).to be true


Consider using:

expect(page(full_file_path("exists.html.erb"))).to be_valid

It's also a little hard to follow along with the number of nested method calls and their opening / closing parens. What do you think about making it easier to read with something like:

page = page(full_file_path("exists.html.erb")) expect(page).to be_valid

throughout.

dgalarza · 2015-05-22T14:42:17Z

@bentoncreation this is looking really good. Just a few more stylistic tweaks and I think this will be ready to merge soon. Thanks for keeping up with it @bentoncreation.

jeremysmithco · 2015-05-29T14:09:57Z

@dgalarza Hey, thanks for the guidance/patience! :) I'm going to try to make your recommended fixes this morning.

dgalarza · 2015-06-05T15:06:39Z

Awesome stuff @bentoncreation. Looks good to me. Would you mind squashing to prep this for a merge? Otherwise I can go ahead and do so.

Also, it might be useful to add something in the README about this and it's applicable usage.

aguynamedben · 2015-06-11T23:17:02Z

👍 nice work @bentoncreation. I'm not familiar with this codebase but I CRed and LGTM. Nice!

jeremysmithco · 2015-06-12T13:50:10Z

Thanks, @dgalarza! I'll work on a README update shortly.

Thanks, @aguynamedben!

jeremysmithco · 2015-06-12T14:32:29Z

Hey @dgalarza, can you let me know if the README update looks ok?

dgalarza · 2015-06-12T17:59:21Z

Looks good to me @bentoncreation

jeremysmithco · 2015-06-13T17:31:16Z

@dgalarza Okay, I think I squashed it right. That was my first time doing an interactive rebase. :) Let me know if anything else is needed.

dgalarza · 2015-06-23T14:38:12Z

Thanks again for the hard work on this @bentoncreation. I'll put out a release with this in it later in the week. 👍

Getting a list of pages (for sitemaps, etc.)

jeremysmithco · 2015-06-23T15:32:52Z

Sounds good, thanks!

houndci-bot reviewed Apr 3, 2015
View reviewed changes

dgalarza reviewed Apr 10, 2015
View reviewed changes

jeremysmithco force-pushed the page_ids branch from c2e7cbd to 8b62bb6 Compare May 20, 2015 17:22

houndci-bot reviewed May 20, 2015
View reviewed changes

dgalarza reviewed May 22, 2015
View reviewed changes

Add page_ids feature

f281912

jeremysmithco force-pushed the page_ids branch from 4d73962 to f281912 Compare June 13, 2015 17:30

dgalarza added a commit that referenced this pull request Jun 23, 2015

Merge pull request #193 from bentoncreation/page_ids

1eba606

Getting a list of pages (for sitemaps, etc.)

dgalarza merged commit 1eba606 into thoughtbot:master Jun 23, 2015

This was referenced Oct 18, 2018

Bump high_voltage from 2.2.1 to 3.1.0 tulibraries/dplah#267

Merged

Bump high_voltage from 2.2.0 to 3.1.0 DefactoSoftware/Hours#435

Closed

dependabot-preview bot mentioned this pull request Apr 18, 2019

Bump high_voltage from 2.2.0 to 3.1.0 juuh42dias/Hours#2

Merged

dependabot-preview bot mentioned this pull request May 27, 2019

Bump high_voltage from 2.2.0 to 3.1.2 DefactoSoftware/Hours#501

Closed

dependabot-preview bot mentioned this pull request Aug 23, 2019

Bump high_voltage from 2.0.0 to 3.1.2 Floppy/Kulaverse#17

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Getting a list of pages (for sitemaps, etc.) #193

Getting a list of pages (for sitemaps, etc.) #193

jeremysmithco commented Apr 3, 2015

houndci-bot Apr 3, 2015

jeremysmithco commented Apr 7, 2015

dgalarza Apr 10, 2015

jeremysmithco Apr 10, 2015

dgalarza commented Apr 16, 2015

jeremysmithco commented Apr 17, 2015

dgalarza commented Apr 17, 2015

jeremysmithco commented Apr 17, 2015

dgalarza commented Apr 17, 2015

jeremysmithco commented Apr 17, 2015

dgalarza commented Apr 17, 2015

jeremysmithco commented Apr 17, 2015

houndci-bot May 20, 2015

dgalarza May 22, 2015

jeremysmithco May 29, 2015

jeremysmithco commented May 20, 2015

dgalarza May 22, 2015

dgalarza May 22, 2015

dgalarza commented May 22, 2015

jeremysmithco commented May 29, 2015

dgalarza commented Jun 5, 2015

aguynamedben commented Jun 11, 2015

jeremysmithco commented Jun 12, 2015

jeremysmithco commented Jun 12, 2015

dgalarza commented Jun 12, 2015

jeremysmithco commented Jun 13, 2015

dgalarza commented Jun 23, 2015

jeremysmithco commented Jun 23, 2015

Getting a list of pages (for sitemaps, etc.) #193

Getting a list of pages (for sitemaps, etc.) #193

Conversation

jeremysmithco commented Apr 3, 2015

Choose a reason for hiding this comment

jeremysmithco commented Apr 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgalarza commented Apr 16, 2015

jeremysmithco commented Apr 17, 2015

dgalarza commented Apr 17, 2015

jeremysmithco commented Apr 17, 2015

dgalarza commented Apr 17, 2015

jeremysmithco commented Apr 17, 2015

dgalarza commented Apr 17, 2015

jeremysmithco commented Apr 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremysmithco commented May 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgalarza commented May 22, 2015

jeremysmithco commented May 29, 2015

dgalarza commented Jun 5, 2015

aguynamedben commented Jun 11, 2015

jeremysmithco commented Jun 12, 2015

jeremysmithco commented Jun 12, 2015

dgalarza commented Jun 12, 2015

jeremysmithco commented Jun 13, 2015

dgalarza commented Jun 23, 2015

jeremysmithco commented Jun 23, 2015