Conversation
lib/readwise/client.rb
Outdated
| res = make_readwise_request(url) | ||
| rescue => exception | ||
| raise exception | ||
| end |
There was a problem hiding this comment.
@andjosh wondering if a begin rescue is the best way to handle errors that might bubble up when we raise them within the enclosed method.
There was a problem hiding this comment.
There's no need to begin/rescue if we're just going to raise again - this can just be: res = make_readwise_request(url)
lib/readwise/client.rb
Outdated
| res = make_readwise_request(url) | ||
| rescue => exception | ||
| raise exception | ||
| end |
There was a problem hiding this comment.
There's no need to begin/rescue if we're just going to raise again - this can just be: res = make_readwise_request(url)
lib/readwise/client.rb
Outdated
| ) | ||
| end | ||
|
|
||
| def make_readwise_request(url) |
There was a problem hiding this comment.
| def make_readwise_request(url) | |
| def get_readwise_request(url) |
I would name this to signify that it's a GET request (because we'll want to support other methods in the future) - or make the HTTP verb a parameter.
lib/readwise/client.rb
Outdated
| http.request(export_req) | ||
| url = BASE_URL + 'export/?' + URI.encode_www_form(params) | ||
|
|
||
| begin |
There was a problem hiding this comment.
Same comment as above, no need to begin/rescue here.
|
|
||
| subject.get_highlight | ||
| end | ||
| end |
There was a problem hiding this comment.
Hopefully this is within the realm of what's needed
There was a problem hiding this comment.
I think we can do better :)
Currently, this test doesn't do anything - it stubs the get_highlight method and a dummy return value and then calls that stubbed method.
I think it would be better to stub out the underlying API call with a mocked HTTP body/response (e.g. https://github.com/andjosh/readwise-ruby/blob/054f040acccdce26ef52d9fbd250995918b1b313/spec/readwise/client_spec.rb#L13-L22) and then test that:
- the method does the validation that a highlight ID is passed as an argument
- The method actually makes the intended underlying API call
- The method correctly parses the HTTP response body and returns an instance of
Highlight
There was a problem hiding this comment.
I think what I just pushed accomplishes at least a bit of what you prescribed, thank you for the suggestions! Writing good tests is something I'm definitely still working on.
|
|
||
| subject.get_highlight | ||
| end | ||
| end |
There was a problem hiding this comment.
I think we can do better :)
Currently, this test doesn't do anything - it stubs the get_highlight method and a dummy return value and then calls that stubbed method.
I think it would be better to stub out the underlying API call with a mocked HTTP body/response (e.g. https://github.com/andjosh/readwise-ruby/blob/054f040acccdce26ef52d9fbd250995918b1b313/spec/readwise/client_spec.rb#L13-L22) and then test that:
- the method does the validation that a highlight ID is passed as an argument
- The method actually makes the intended underlying API call
- The method correctly parses the HTTP response body and returns an instance of
Highlight
|
|
||
| subject.get_highlight | ||
| end | ||
| end |
|
Published in v0.4.0 |
2 participants
Aims to take care of #9