Finish first 7 endpoints + unit tests + crate docs #2
Conversation
wenbinf
left a comment
wenbinf
left a comment
There was a problem hiding this comment.
Overall, we are on the right track.
Please address my comments. I believe after one more iteration, we could merge this pull request and move on to next one :)
Thanks!
src/client.rs
Outdated
| pub async fn episode_by_id(&self, id: &str, parameters: &Value) -> Result<Value> { | ||
| self.get(&format!("episodes/{}", id), parameters).await | ||
| /// Calls [`GET /best_podcasts`](https://www.listennotes.com/api/docs/#get-api-v2-best_podcasts) with supplied parameters. | ||
| pub async fn best_podcasts(&self, parameters: &Value) -> Result<Value> { |
There was a problem hiding this comment.
let's use the same naming convention as in other sdks -
https://github.com/ListenNotes/podcast-api-python/blob/main/listennotes/podcast_api.py#L42
For example, this method should be fetch_best_podcasts
There was a problem hiding this comment.
Ahh, didn't realize all the libs shared that naming scheme, will update this one to match.
src/client.rs
Outdated
| } | ||
|
|
||
| /// Calls [`GET /podcasts/{id}`](https://www.listennotes.com/api/docs/#get-api-v2-podcasts-id) with supplied parameters. | ||
| pub async fn podcast(&self, id: &str, parameters: &Value) -> Result<Value> { |
src/client.rs
Outdated
| pub async fn genres(&self, parameters: &Value) -> Result<Value> { | ||
| self.get("genres", parameters).await | ||
| /// Calls [`GET /episodes/{id}`](https://www.listennotes.com/api/docs/#get-api-v2-episodes-id) with supplied parameters. | ||
| pub async fn episode(&self, id: &str, parameters: &Value) -> Result<Value> { |
src/client.rs
Outdated
| } | ||
|
|
||
| /// Calls [`POST /episodes`](https://www.listennotes.com/api/docs/#post-api-v2-episodes) with supplied parameters. | ||
| pub async fn episodes(&self, parameters: &Value) -> Result<Value> { |
| } | ||
|
|
||
| #[test] | ||
| fn search() { |
There was a problem hiding this comment.
maybe add a test_* prefix to each unit test name? Making this test_search()
not sure if it's conventional to have both unit test & the tested function have the same name.
There was a problem hiding this comment.
I wouldn't say there's widespread consensus on this but this way of doing it isn't uncommon since it's already in a test file and you don't need to worry about name/symbol collision. This is how I normally do it but it's largely personal opinion so I'll leave it up to you if you want these as X or test_X. Happy to change them if you want
|
|
||
| #[test] | ||
| fn search() { | ||
| let response = b!(client().search(&json!({ |
There was a problem hiding this comment.
Let's test 4 things for each client function:
1, see if the endpoint url is correct (sometimes we copy & paste to make a function hit the wrong url)
2, see if the http method is correct. For this function, it should be GET; for some other functions, it should be POST or DELETE
3, see if parameters are passed okay. For GET functions, check if query strings contain the expected parameters.
4, see if the response json is good, which you already did.
Example: https://github.com/ListenNotes/podcast-api-python/blob/main/tests/test_client.py#L16
There was a problem hiding this comment.
Hmm, this would be a bit awkward to add to the existing tests as I don't like any of the solutions for making that information user facing but I could test those items separately with a module mocking your API expecting those properties from the client.
There was a problem hiding this comment.
let's make a new Response struct similar to the one in our swift sdk:
https://github.com/ListenNotes/podcast-api-swift/blob/main/Sources/PodcastAPI/ApiResponse.swift#L5
We need users to be able to access stats returned from response headers and maybe the raw request object.
Let me know if you have any questions about this update.
Take a look at the crate docs too (what goes on docs.rs when you publish). You can run
cargo doc --opento build and open the html docs locally. I'm sure you might want to tweak them so let me know if you have any questions about how the commenting system works. In general, triple///comments above a public item are treated as markdown for the corresponding item in the doc pages. The//!are module level docs that just show up at the top of the page for that module, also in markdown.cargo testalso runs anything you have in markdown code blocks in the crate docs so the doc tests will fail if the library is updated and the documentation comments aren't, which is nice for making sure things aren't out of date.