Fix dead links #94

Merged
merged 9 commits into from Jan 14, 2013

Conversation

Projects
None yet
2 participants
Owner

georgebrock commented Jan 10, 2013

I noticed a dead link this morning, and while fixing it I noticed many previous pull requests were fixes for dead links, so I added some code to check for them.

@@ -4,6 +4,8 @@ ruby '1.9.3'
gem 'bourne'
gem 'colored'
+gem 'httparty'
gem 'json'
gem 'mocha', require: false
@adarsh

adarsh Jan 10, 2013

Contributor

Mocha will throw deprecation warnings unless you put it last in the gem list. Maybe with a comment like

gem 'mocha', require: false   #this gem needs to be last in the list

To prevent future undesired alphabetization (by me :) )

@georgebrock

georgebrock Jan 11, 2013

Owner

The order of the Gemfile didn't get rid of the deprecation notice for me. Nor did removing Mocha and manually requiring Bourne (see https://github.com/thoughtbot/bourne/issues/14)

lib/helpers/uri_extractor.rb
+ def initialize(file_name)
+ @file_name = file_name
+ contents = File.read(@file_name)
+ @json = JSON.parse(contents)
@adarsh

adarsh Jan 10, 2013

Contributor

This stuff is truly awesome.

Minor point, but how do you feel about extracting the parsing to a private method also? Something like

class URIExtractor
  def initialize(file_name)
    @file_name = file_name
    @json = parse_json
  end

  # ...

  private

  def parse_json
    JSON.parse(File.read(@file_name))
  end
@georgebrock

georgebrock Jan 11, 2013

Owner

Looks good. Done.

lib/helpers/uri_validator.rb
+ if response.method_not_allowed?
+ response = HTTParty.get(uri)
+ end
+ return response.ok?
@adarsh

adarsh Jan 10, 2013

Contributor

Minor Style: Newlines preferred around blocks.

@georgebrock

georgebrock Jan 11, 2013

Owner

I've altered the behaviour of this method slightly to handle errors more robustly, and the block went away in the process.

Contributor

adarsh commented Jan 10, 2013

AMAZING. I've been wondering how something like this would be done. Thanks for submitting this.

Unclear how much people would use it, but this would be a cool gem. Seems like there are a lot of choices for static http pages, but none for JSON.

A+ 🌈 Ready to merge.

georgebrock added some commits Jan 11, 2013

Fallback to GET on all errors.
Some sites don't respond to HEAD requests at all, which gives false negatives.
Owner

georgebrock commented Jan 11, 2013

Changes made, ready for re-review.

@@ -60,7 +60,7 @@
},
{
"title": "Visit 'Information Design' on Learn",
- "uri": "https://learn.thoughtbot.com/information+design"
+ "uri": "https://learn.thoughtbot.com/information-design"
@georgebrock

georgebrock Jan 11, 2013

Owner

This link is still dead; information design isn't a featured topic on learn.thoughtbot.com. I'm not sure if it's better to remove this link or add the topic to learn?

Contributor

adarsh commented Jan 12, 2013

Looks good 🍫

Unclear about the information-design link. It's definitely a trail.

Maybe add an issue on Learn?

@georgebrock georgebrock merged commit 3374719 into master Jan 14, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment