Skip to content

Fix :head option for Pods from HTTP source. #1958

Merged
merged 4 commits into from Mar 30, 2014

2 participants

@neonichu
CocoaPods member

HTTP doesn't implement checkout_options.

While specifying :head for Pods from HTTP sources doesn't really make sense, it feels like it should still work so that the author of a Podfiledoesn't have to know about the download source of each Pod he uses.

@alloy
CocoaPods member
alloy commented Mar 29, 2014

I don’t think it should work, as they might then (obviously incorrect anyways) wonder why it doesn’t update. I think that instead the error message should be informative. E.g. The pod 'spatialite' is downloaded from a HTTP source, which does not support the:head' option. It only applies to SCM systems.`

(Make the message a bit extra nice, I just quickly typed something ;))

@neonichu
CocoaPods member

True, such an error message would be better. Also a bigger undertaking, because first cocoapods-downloader would need to provide that information.

@alloy
CocoaPods member
alloy commented Mar 29, 2014

We don’t need to modify the exception in cocoa pods-downloader (although that one could be prettier too), but instead you can do something like:

if downloader.options_specific?
  @specific_source = downloader.checkout_options
else
  raise Informative, "Better error message."
end
@neonichu
CocoaPods member

OK, will take a stab at that.

@neonichu
CocoaPods member

It's not that easy, as options_specific will be false for Git as well, if no explicit commit or tag is specified. We could check if checkout_options raises but that seems very unclean to me.

@alloy
CocoaPods member
alloy commented Mar 29, 2014

Ah good point.

We could check if checkout_options raises but that seems very unclean to me.

While it often is, I’m not sure right now what the best way would be to implement it. So how about rescuing and raising for now and filing a ticket on cocoapods-downloader to allow for nicer error handling?

begin
  # do work
rescue RuntimeError => e
  if e.message == 'Abstract method'
    raise Informative, "Does not support #{options}"
  else
    raise
  end
end
@neonichu
CocoaPods member

OK, makes sense. In addition to being unclean, it has the disadvantage that it will first download, then fail :)

@neonichu neonichu referenced this pull request in CocoaPods/cocoapods-downloader Mar 30, 2014
Closed

Provide information if a certain downloader supports :head #12

@alloy
CocoaPods member
alloy commented Mar 30, 2014

Very true, the more reason we should fix this properly. But I do believe that the time it takes for a user to download an archive is nothing compared to the rage unleashed by having to debug ;)

@alloy
CocoaPods member
alloy commented Mar 30, 2014

Sorry for not making this note earlier, but the change also needs an accompanying test case :) Let me know if you need assistance with that.

@neonichu
CocoaPods member

Of course I do :). Have added one, probably bad, though, because it will actually hit HTTP?

@alloy
CocoaPods member
alloy commented Mar 30, 2014

Thanks! When I get back, later today, I will update the test to stub it so it doesn't actually try to download.

And finally, can you also add a CHANGELOG entry to credit yourself?

@neonichu neonichu added a commit to neonichu/CocoaPods that referenced this pull request Mar 30, 2014
@neonichu neonichu Updated CHANGELOG for #1958 6a24a78
@neonichu
CocoaPods member

Updated CHANGELOG in #1972

@neonichu
CocoaPods member

OK, figured out how to do that myself :)

@alloy alloy merged commit 82e0afa into CocoaPods:master Mar 30, 2014
@neonichu neonichu deleted the neonichu:fix_head_download_option branch Mar 30, 2014
@alloy
CocoaPods member
alloy commented Mar 30, 2014

Perfect, thanks! (I amended the patch slightly because it was clashing with another change that cleans a dir when an error occurs.)

@neonichu
CocoaPods member

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.