search

ropensci / patentsview

An R client to the PatentsView API
https://docs.ropensci.org/patentsview
Other
31 stars 9 forks source link

X-Status-Reason display and location by group test #12

Closed mustberuss closed 6 years ago

mustberuss commented 6 years ago

I added code to show the X-Status-Reason header returned by the api on 400 and 500 errors. I also added a test to retrieve locations data by group. Attached is a list of 400 and 500 errors generated by the api. errors.txt

crew102 commented 6 years ago

Hi @mustberuss, thanks for the PR. A few notes:

  1. In the future, can you please first create an issue (if one doesn't already exist) before submitting a PR? For example, you could have created an issue like "patentsview should throw the full status-reasons when API throws an error." This allows discussion of the issue before you start working on a PR. Also, please try to limit PRs to a single issue (e.g., 1 issue/PR for dealing with x-status-codes, and another issue/PR for adding additional tests of the locations endpoint).

  2. Re: using the x-status-code reasons, this is a good idea. The API used to send error details in the form of HTML messages, which patentsview would allow the user to browse view the error_browser option to search_pv(). However, it looks like the API is now sending that info as detailed x-status-codes instead of HTML. I'l refactor the code in R/process-error.R (e.g, drop the custom_er() function) after I pull in your changes .

  3. Don't worry about the failing tests on travis for now. The reason why travis is failing is related to https://github.com/CSSIP-AIR/PatentsView-API/issues/28, while appveyor is passing b/c of #13.

  4. Before I forget, I know you have been looking over then API's code and I wanted to make sure you saw that a good amount of work seems to have been done outside master at https://github.com/CSSIP-AIR/PatentsView-API/tree/export-branch. I hadn't noticed this until just now.

I'll merge/refactor sometime this week. Thanks again.

mustberuss commented 6 years ago

@crew102, I should have included a better description. Both changes would be for issue #11 The test was what I did locally to check that requests to the location endpoint work for a single group at a time. Collectively it gives testing coverage of the possible locations fields which isn't currently possible when every field is requested. The x-status-code could be checked to identify that the 500's cause was 'Query execution failed.', presumable because too many groups were requested. I had a different custom message coded locally before I noticed you had already coded it. The header check was the only functional difference.

It is encouraging that the api development is ongoing. It's frustrating though how slow they are to reply in their own forum and that they haven't even acknowledged the issues you've opened.