Filter endpoint tests, unittest asserts, pylint cleanups (Closes #45) #60
Conversation
Well, I know Gramps uses only Running plain But it's true that |
| @@ -74,6 +78,8 @@ def get_custom_filters(args: Dict[str, str], namespace: str) -> List[Dict]: | |||
| ], | |||
| } | |||
| ) | |||
| if "filters" in args and len(args["filters"]) != len(filter_list): | |||
| abort(404) | |||
There was a problem hiding this comment.
Why 404 and not 400, isn't this an invalid request?
There was a problem hiding this comment.
My thinking is that it did not find all the filters in the list because one was missing, and we would provide 404 for a missing object. I did the same for rules just above and do the same below in build filter.
gramps_webapi/api/__init__.py
Outdated
| @@ -68,7 +68,8 @@ def register_endpt(resource: Type[Resource], url: str, name: str): | |||
| register_endpt(BookmarkResource, "/bookmarks/<string:namespace>", "bookmark") | |||
| register_endpt(BookmarksResource, "/bookmarks/", "bookmarks") | |||
| # Filter | |||
| register_endpt(FilterResource, "/filters/<string:namespace>", "filter") | |||
| register_endpt(FilterResource, "/filters/<string:namespace>/<string:name>", "filter") | |||
| register_endpt(FilterResource, "/filters/<string:namespace>", "filters") | |||
There was a problem hiding this comment.
So now you have multiple endpoints for the same resource class and the different verbs will have different number of arguments. Does that work? What if I send a GET request to /filters/namespace? Won't that cause a 500 server error? Don't you have do define name as a keyword argument (with default value) on all the verbs' methods? And return 405 if it's set on the wrong method?
There was a problem hiding this comment.
Oh yeah, you're right, I'm not handling that right. I will fix that up, thanks!
There was a problem hiding this comment.
Rebased as well. I think that is a bit better now. Boy I miss the obvious sometimes.
Yeah, this is my first real experience writing extensive test cases so I don't really know the pros and cons of each framework and approach. I'm hoping what I did for the test cases overall is okay, it made sense to me but you are far more experienced than I. |
|
If this looks okay now could you consider merging it so I can rebase the others? Or merge one of the others so I can rebase what is left? Or do you want me to combine a couple of them? |
|
I am unalterably and intensely opposed to having anything even vaguely resembling a web API in GnuCash, and even more opposed to one written by the Guile developers. That is a security hole so huge that no responsible development team without the requisite time and skills could foist upon its users. If you want to support such a thing then write it as a plugin and host it yourself. |
Hi John, I am a little confused by this statement, what does GnuCash have to do with Gramps? Did you mean to say you are opposed to a REST API for Gramps? |
|
I'm very sorry, I confused this PR with a GnuCash one also by someone named Christopher who has a habit of proclaiming his PRs ready to merge when they aren't. Please disregard the noise. |
|
Let's merge this before more opposition arises 😉 |
@DavidMStraub this actually touches things in five areas, which I know you won't be thrilled about, but I just struggled with doing things one way for
test_filters.pyand not fixing the other test cases I put together to be conformant.Changes
/api/filtersendpoint so delete now follows the more standard convention of having the item in the path as well as tweaks the two query parms on a get andapispec.yamlupdate.The remainder of the
/api/filterstests intest_filters.py. Those cover create / update / apply / delete of a custom filter for each endpoint.It replaces all of the plain
assertsin the test_endpoints modules with the more specific unittest ones likeassertEqual,assertTrue, andassertInwhich I realized I really should have done from the start.It renames
rvtoresultin all the test_endpoints modules along with other changes to try to appease pylint as best I could.It includes two changes to
.pylintrcthat I had to make as it refused to run at all on my machine. The first removes the duplicate [MESSAGES CONTROL] section at the bottom and the second changes fail-under=10.0 to fail-under=10 as pylint wanted it to be an integer. I am unsure on this one, it clearly must be working on your system. I'm using 2.6.0.Sorry again for the size. I know you are working on test cases too and may be some collision there. So if you're stuff is ready to merge maybe do that first and I can rebase and revise this. Let me know your thoughts.