Support Typhoeus options from command line #490

SeanKilleen · 2018-09-15T05:10:07Z

Hi all,

First off -- thanks for a great tool! It's useful enough that I went out of my way to use it for my Jekyll blog recently.

I noticed that there appear to have been some requests previously to support Typhoeus options from the command-line. I would also find this quite useful, even though we can currently use a ruby script to obtain the same things.

I figured I would take a shot at contributing this feature as an exercise to expand my own ruby capabilities as well as contribute something back that I and others have expressed interest in.

Along the way, I am open to guidance and direction. And I intend to ensure the specs are covered to your satisfaction.

Suggested Feature Set (as I understand it)

Enable users to pass --typhoeus-config CONFIG with a JSON-formatted config string which we then pass directly in as typhoeus options to the original options object. This is then merged with the defaults.

NOTE: I've examined proposing this with both a command line and a config file option, but am choosing to limit this PR to the cmdline option (will do a follow-up PR for allowing a file if the support is there for adding it).

Example Usage

bundle exec htmlproofer --typhoeus-config '{ "timeout": 100 }' ./_site

To Do (as I understand it)

Specs

When JSON parsing is invalid (for file & direct input), show a graceful error
When no Typhoeus config specified, should be equal to the defaults
When Typheous config specified, overwrites the defaults.

SeanKilleen · 2018-09-15T05:24:57Z

As of this point, when testing locally, the following command works as expected:

ruby htmlproofer --typhoeus-config '{ "timeout": 100, "verbose": true }' [directory I'm testing]

I see a longer timeout and verbose output.

So it does appear that this is parsing the JSON, passing it through, and mapping it on top of the defaults that html-proofer is supplying.

I know it's a far cry from being ready to go yet, but that result is encouraging.

SeanKilleen · 2018-09-15T05:46:45Z

I noticed that the Travis CI build failed. Upon inspection, those appear to be two unrelated rubocop findings in areas I haven't modified. If you believe it's something to do with this PR, let me know and I'll be happy to modify.

gjtorikian · 2018-09-17T21:31:06Z

Hey, this is looking good!

I noticed that the Travis CI build failed. Upon inspection, those appear to be two unrelated rubocop findings in areas I haven't modified.

Thanks for that report. I merged a fix into master: #491

If you update your branch, the tests should pass.

I think the only thing keeping this from being merged in, really are spec tests; I'd also really like to see a rescue for the case where JSON.parse might be fed a non-JSON string. This should report as an error back to the CLI.

SeanKilleen · 2018-09-18T03:30:11Z

@gjtorikian thanks for the feedback and being receptive to the PR! At first glance the spec updates seem doable for sure, just going to take a little time with it due to less familiarity with Ruby in general. I figure I'll do a few and then you can suggest others I may have missed?

SeanKilleen · 2018-09-23T16:08:15Z

@gjtorikian hey! I'm looking into this and I don't see any specs set up specifically around the mercenary configuration and testing options, etc. I started testing check_file by passing in options but I realized that's later in the pipeline than is effective to test.

Am I missing the location of those specs? If they don't exist, would you prefer I:

Look into mercenary and try to create a spec around the passing of options?
Add the rescue clause and provide some screenshots of the outputs on my local?

Normally the first option would be my preference, but given the lack of familiarity I wonder if that's a heavy lift for this PR given my familiarity. Happen to take a stab at it if you prefer though.

SeanKilleen · 2018-09-23T16:33:57Z

@gjtorikian I extracted the functionality into a new function that I'm hoping I can write specs around. Pushed some changes including empty specs and the beginnings of the refactor. Look good?

codecov-io · 2018-09-23T17:32:23Z

Codecov Report

Merging #490 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #490      +/-   ##
==========================================
+ Coverage   98.27%   98.31%   +0.03%     
==========================================
  Files          27       28       +1     
  Lines        1802     1838      +36     
==========================================
+ Hits         1771     1807      +36     
  Misses         31       31

Impacted Files	Coverage Δ
lib/html-proofer.rb	`100% <100%> (ø)`	⬆️
spec/html-proofer/parse_json_option_spec.rb	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1ada79...a2d322f. Read the comment docs.

SeanKilleen · 2018-09-23T17:59:02Z

Apologies if you're getting push notifications on this -- I'm doing my dev in windows for convenience and need to push the changes so I can test them in my Ubuntu WSL instance, so there's some noise as I work through stuff.

I'll definitely be squashing these changes.

SeanKilleen · 2018-09-23T19:01:58Z

@gjtorikian I think this might be ready to go -- as far as I can tell, all the related specs pass on my local (one unrelated spec is failing but appears to be to due more with a timeout).

I rebased to squash & clean stuff up as well. Would love your review when you get the chance!

SeanKilleen · 2018-10-06T03:14:06Z

@gjtorikian I think this is ready to go, so just checking in. The build is failing but my understanding is it's an unrelated timeout on an existing case and not due to my changes. I'll double-check to make sure.

Update: nevermind I can see the build output now and I see there's some rubocop violations to clean up. working on it now!

SeanKilleen · 2018-10-06T03:54:27Z

@gjtorikian OK, I fixed all the rubocop issues and then rebased to squash / include them all in the larger commit. Now truly green & passing. ✅

SeanKilleen · 2018-10-10T20:57:17Z

@gjtorikian wanted to check in on this. I'm sure you're probably busy, but I'd love to get it in soon or shore it up if you feel it's missing something. Or if it needs to wait for a bit, let me know and I'll stop bugging you. :)

Build passes
Rubocop issues fixed
Specs exist (at least as many as I can think of)
Local usage checks out on my WSL install

SeanKilleen · 2018-10-24T00:52:19Z

@gjtorikian any word on this?

Floppy

This looks good to me!

SeanKilleen · 2018-11-21T15:41:48Z

Thanks @Floppy!

@gjtorikian let me know if you think anything else is needed here.

SeanKilleen · 2018-12-07T18:15:50Z

@gjtorikian checking in one more time on this.

✅ Build passing
✅ Additional tests created & passing
✅ Local usage confirms it works as intended as well
✅ Positive feedback received from you in Sept.
✅ Approved by @Floppy on Nov 19

Are there any hesitations I could address? Or would it be possible to get this merged soon? Happy to address any questions or concerns.

gjtorikian · 2019-01-04T19:35:59Z

Hi, I'm very sorry for my delay in review here for this much requested feature! Personal life got in the way of open source, I'm afraid.

This looks nearly perfect--I simply moved the parse_json_option into the Configuration module, and changed up the syntax a bit.

Thank you so much for this contribution! And again, I'm sorry it took me so long to get back to you.

SeanKilleen · 2019-01-04T19:48:13Z

No worries at all! I totally understand and I'm all about work-life balance. 👍 Glad this is useful and was able to make it in!

SeanKilleen requested a review from Floppy as a code owner September 15, 2018 05:10

SeanKilleen force-pushed the allow-typhoeus-options branch from d7f1b67 to 6b1aea7 Compare September 23, 2018 19:00

SeanKilleen changed the title ~~In Progress: Support Typhoeus options from command line~~ Support Typhoeus options from command line Sep 23, 2018

Add JSON option parsing functionality

a2d322f

SeanKilleen force-pushed the allow-typhoeus-options branch from 30cf70d to a2d322f Compare October 6, 2018 03:51

Floppy approved these changes Nov 19, 2018

View reviewed changes

gjtorikian merged commit a2d322f into gjtorikian:master Jan 4, 2019

SeanKilleen mentioned this pull request Jan 4, 2019

command line ssl_verifypeer false #194

Closed

patcon mentioned this pull request Feb 13, 2021

Added data attribute to ignore twitter link, which now throws 400 from CLI hyphacoop/handbook#110

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Typhoeus options from command line #490

Support Typhoeus options from command line #490

SeanKilleen commented Sep 15, 2018 •

edited

Loading

SeanKilleen commented Sep 15, 2018 •

edited

Loading

SeanKilleen commented Sep 15, 2018

gjtorikian commented Sep 17, 2018

SeanKilleen commented Sep 18, 2018

SeanKilleen commented Sep 23, 2018

SeanKilleen commented Sep 23, 2018

codecov-io commented Sep 23, 2018 •

edited

Loading

SeanKilleen commented Sep 23, 2018 •

edited

Loading

SeanKilleen commented Sep 23, 2018

SeanKilleen commented Oct 6, 2018 •

edited

Loading

SeanKilleen commented Oct 6, 2018

SeanKilleen commented Oct 10, 2018

SeanKilleen commented Oct 24, 2018

Floppy left a comment

SeanKilleen commented Nov 21, 2018

SeanKilleen commented Dec 7, 2018

gjtorikian commented Jan 4, 2019

SeanKilleen commented Jan 4, 2019

Support Typhoeus options from command line #490

Support Typhoeus options from command line #490

Conversation

SeanKilleen commented Sep 15, 2018 • edited Loading

Suggested Feature Set (as I understand it)

Example Usage

To Do (as I understand it)

Specs

SeanKilleen commented Sep 15, 2018 • edited Loading

SeanKilleen commented Sep 15, 2018

gjtorikian commented Sep 17, 2018

SeanKilleen commented Sep 18, 2018

SeanKilleen commented Sep 23, 2018

SeanKilleen commented Sep 23, 2018

codecov-io commented Sep 23, 2018 • edited Loading

Codecov Report

SeanKilleen commented Sep 23, 2018 • edited Loading

SeanKilleen commented Sep 23, 2018

SeanKilleen commented Oct 6, 2018 • edited Loading

SeanKilleen commented Oct 6, 2018

SeanKilleen commented Oct 10, 2018

SeanKilleen commented Oct 24, 2018

Floppy left a comment

Choose a reason for hiding this comment

SeanKilleen commented Nov 21, 2018

SeanKilleen commented Dec 7, 2018

gjtorikian commented Jan 4, 2019

SeanKilleen commented Jan 4, 2019

SeanKilleen commented Sep 15, 2018 •

edited

Loading

SeanKilleen commented Sep 15, 2018 •

edited

Loading

codecov-io commented Sep 23, 2018 •

edited

Loading

SeanKilleen commented Sep 23, 2018 •

edited

Loading

SeanKilleen commented Oct 6, 2018 •

edited

Loading