add new CurlPostDownloadStrategy

[pedros]

• lib/cask/url.rb: initialize(): add new :data attribute to hold post parameters
• lib/cask/download.rb: perform(): dispatch to new class based on :using => :post
• lib/cask/download_strategy.rb: Cask::CurlPostDownloadStrategy:
  extend curl_args with x-www-form-urlencoded data
• doc/CASK_LANGUAGE_REFERENCE.md: HTTP URLs: document new strategy

[pedros]

It would be neat to have the possibility of POSTing forms that then redirect to the correct url. A lot of vendors appear to only offer this possibility. This patch helps address that problem.

[rolandwalker]

Thanks for this! I'll review it tomorrow.

Do you have an example Cask where this would be used?

[pedros]

Yes I do. Would you like me to push it here, or to a new branch and separate PR?

[pedros]

Sorry, I rewrote my branch to squash a separate test commit into the main one. If you've pulled from my branch to test locally, please pull again cleanly.

[pedros]

Please see #3423 for use case.

[rolandwalker]

You added tests? Great!

Yes, it is better to keep the Cask in a separate PR, so that users don't pull in a broken Cask via brew update during the interval between merge and tagging a release.

[rolandwalker]

The code looks great.

I'm trying to figure out why Homebrew implements this in much fewer lines of code (https://github.com/Homebrew/homebrew/blob/master/Library/Homebrew/download_strategy.rb#L264). However, there don't seem to be any Homebrew Formulae which actually use :post. And @phinze has requested that we move toward having our own independent implementation here.

I assume the curl_args method could be simplified by calling super?

Also, post_args could be simplified by dropping the else, implying a nil result for empty, something like (untested)

def post_args
  @post_args ||= if cask_url.data
    # sort_by is for predictability between Ruby versions
    cask_url.data.sort_by{ |key, value| key.to_s }.map do |key, value|
      ['-d', "#{CGI.escape(key.to_s)}=#{CGI.escape(value.to_s)}"]
    end
  end
end

Elsewhere, we would say, for example,

args.concat(post_args) if nil

These are just points for discussion; please don't think that addressing every item is a requirement for inclusion of this very useful code.

[pedros]

I did notice Homebrew's own CurlPostDownloadStrategy implementation: it basically splits the url on the ? character, and passes that to curl's -d option. I wanted to provide an interface similar to the :cookies one, so I decided on what you see here.

I simplified curl_args as you suggested. In post_args I just copied the style prevalent in the file: that included the dangling else returning an empty array. However, I was missing the case where people want to simply POST and not pass in any parameters, so I ended up adding the following:

else
  ['-X', 'POST']
end

Another thing: I had to change the relevant test, because includes_args only tests set membership, which breaks on my previous test with multiple -d options, since set intersection on ['-d', 'coo=kie', -d 'mon=ster'] with the arguments array returns ['-d', 'coo=kie', 'mon=ster']. Finally, I added another test, for the -X POST option.

[rolandwalker]

Yes, your interface is far better than hacking the URL string.

I'm weak on the testing infrastructure in Ruby, will study your changes/notes with interest. Will check out this branch again tomorrow and run the tests.

In my view, this is ready to merge. But let's leave the PR open for a few days in case anyone cares to add to the discussion.

[rolandwalker]

Merging!

If you feel like switching places, and if you have the time, I'd love to have your review on #3066 and #3043.

The last one is still a work-in-progress, and will probably have a conflict after I merge this one.

[pedros]

Great! Thanks for your efforts. I will have a look at those PRs later tonight.