Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds protocol support to make-domain-requester #22

Merged
merged 4 commits into from
Aug 26, 2016

Conversation

DarrenN
Copy link
Contributor

@DarrenN DarrenN commented Aug 21, 2016

When setting up a domain-requester you can pass the optional #:protocol keyword to enable HTTPS URLs. The default remains HTTP.

Example:

(make-domain-requester "httpbin.org" http-requester #:protocol "https")

@DarrenN
Copy link
Contributor Author

DarrenN commented Aug 21, 2016

Hmmm, the tests pass but there seems to be an issue with coveralls?

@jackfirth
Copy link
Owner

Rather than adding an option to make-domain-requester, how about adding a separate make-https-requester procedure that wraps a requester and transforms the urls it uses to https urls? That makes it easier to add https to requesters created via other means.

@jackfirth
Copy link
Owner

Not sure why Coveralls is failing, and the Travis racket versions need to be updated anyways. I'll look into that next time I get a chance.

@DarrenN
Copy link
Contributor Author

DarrenN commented Aug 23, 2016

@jackfirth that makes sense. Pushed up a new commit adding make-https-requester.

(wrap-requester-location
(domain+relative-path->http-url "http" domain _) requester))

(define (make-https-requester domain requester)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think make-https-requester shouldn't work with a domain and should only change the URL of requests made with the underlying requester to HTTPS. Something like (wrap-requester-location http-url->https-url requester).

@DarrenN
Copy link
Contributor Author

DarrenN commented Aug 23, 2016

Okay I think this follows along with what you suggested:

(make-domain-requester domain (make-https-requester http-requester))

Also updated the tests and added the integration-test submodule. I think I correctly modified the .travis.yml file as well.

@@ -15,5 +15,5 @@ before_install:
install: raco pkg install --deps search-auto $TRAVIS_BUILD_DIR

script:
- raco test $TRAVIS_BUILD_DIR
- raco test --submodule integration-test $TRAVIS_BUILD_DIR
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need two test commands, the original one that runs unit tests and this one to run integration tests. Otherwise any (module+ test body ...) forms elsewhere in the package won't run. I haven't actually written any tests though, so technically not a big deal.

@jackfirth
Copy link
Owner

This looks great 👍 Thanks for sticking with it through review, and thanks for implementing this! Would you be willing to write some docs for it? I'll try and fix the travis/coveralls issues tonight.

@DarrenN
Copy link
Contributor Author

DarrenN commented Aug 23, 2016

This has been great, a nice lesson in composition for sure. Will write up some docs tonight. I've also been working on a json-requester branch, will review that some more in light of this discussion before I PR it.

Thank again!

@jackfirth
Copy link
Owner

If you'd like I can assign #12 to you then, json requesters has been on my list for some time.

@DarrenN
Copy link
Contributor Author

DarrenN commented Aug 23, 2016

Pushed up docs for make-https-requester. Sure I'll take a swing at #12

@jackfirth
Copy link
Owner

Merged #23, fixed cover and now it builds on more versions. You'll have to rebase.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 84.681% when pulling 8c3b163 on DarrenN:add-protocol into 537438f on jackfirth:master.

@DarrenN
Copy link
Contributor Author

DarrenN commented Aug 26, 2016

Nice to see the tests run against so many different Rackets.

@jackfirth jackfirth merged commit 49936b2 into jackfirth:master Aug 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants