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

Added Utils.resolve_url and some tests #14

Merged
merged 2 commits into from
Aug 12, 2013

Conversation

dannguyen
Copy link
Contributor

Added a way to deal with relative urls and a slightly-altered test in the upton_spec suite to show that it works. However, I think in the long run, some changes will have to be made to the Upton API, which currently assumes a single kind of @index_url (as an instance variable created at initialization).

There may be some extreme edge cases that resolve_url doesn't work on. Compare its shortness versus the version by Mechanize:

https://github.com/sparklemotion/mechanize/blob/5ac3970a1b1475e8325bbf29b54abf0f7b3ecb60/lib/mechanize/http/agent.rb

@dannguyen dannguyen mentioned this pull request Aug 9, 2013
@jeremybmerrill
Copy link
Contributor

Looks awesome. I want to take a closer look, but I expect to merge this soon.

Also, as I said, I'm not an RSpec expert, by any means. Is what you've got in spec/upton_spec.rb in 4397c28 the idiomatic RSpec way to write as-yet-unwritten tests? (e.g. just it "should do whatever" without a following block).

@dannguyen
Copy link
Contributor Author

That seems to be the easiest way. If you want to leave more explanation why, you can also throw in the block with a pending

  it 'has to do this' do 
    pending 'for some reason'
  end

@dannguyen
Copy link
Contributor Author

Also, hopefully the test suite won't require anyone here to be an RSpec framework ;). I switched to it from MiniTest...the syntax is only slightly different and it provides a lot of nice conveniences, including at the commandline (running single test files, or single tests, or testnames that match a regex) and apparently is simpler to mock things with out of the box (never have used many Mocks myself though). I don't think the speed advantage of MiniTest will matter much given the relatively few tests that this should all need

@kgrz
Copy link
Contributor

kgrz commented Aug 11, 2013

@dannguyen Major plus is the availability of plugins I suppose. And there would definitely be more resources for rspec than MiniTest/MiniTestSpec. :)

@jeremybmerrill jeremybmerrill merged commit 7c475a3 into propublica:master Aug 12, 2013
@jeremybmerrill
Copy link
Contributor

Merged just now, for 0.2.7.

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.

3 participants