Reduce cognitive complexity and typo fixes #1457
Conversation
…uce cognitive complexity
… multiple times + in parallel in one shot
|
|
||
| if middleware is None: | ||
| middleware = graphene_settings.MIDDLEWARE | ||
| if isinstance(middleware, MiddlewareManager): |
There was a problem hiding this comment.
Let me know if I should've kept the check for middleware is not None here - my thought process was that we already check above if it's none, so probably we don't need to check if now it is not none.
Unless graphene_settings.MIDDLEWARE can be None - then this is needed and I can re-add it back in.
There was a problem hiding this comment.
Thanks for making improvements throughout graphene-django! Would it be possible to break this up into a couple different PRs: one with just the typo/content fixes, and one with actual code changes? I haven't taken the time to go through meticulously and validate that the refactored code matches the original behavior, and personally less sure how necessary that is (I think complexity scores only tell you so much). The typo fixes and whatnot seem like an easy thing to merge first. (On the code changes, I just left a couple comments where there are breaking changes to method names, which I don't think we should merge as is. If there are other breaking changes to graphene-django method signatures we should avoid those as well.)
| @@ -30,7 +30,7 @@ class ArticleConnection(Connection): | |||
|
|
|||
| test = String() | |||
|
|
|||
| def resolve_test(): | |||
| def resolve_test(self): | |||
There was a problem hiding this comment.
The convention for the resolve_* methods for an ObjectType (which Connection is) is to use parent as the first argument, not self, since these are "implicit static methods": https://docs.graphene-python.org/en/latest/types/objecttypes/#naming-convention
| @@ -128,7 +128,7 @@ def _client(self, client): | |||
| ) | |||
| self.client = client | |||
|
|
|||
| def assertResponseNoErrors(self, resp, msg=None): | |||
| def assert_response_no_errors(self, resp, msg=None): | |||
There was a problem hiding this comment.
This is a breaking change for consumers of this package. I would recommend at least preserving an alias with the original camelCase name. I imagine it was named this way for consistency with Django (and Python's) camelCase assertion methods: https://docs.djangoproject.com/en/4.2/topics/testing/tools/#assertions
| @@ -138,7 +138,7 @@ def assertResponseNoErrors(self, resp, msg=None): | |||
| self.assertEqual(resp.status_code, 200, msg or content) | |||
| self.assertNotIn("errors", list(content.keys()), msg or content) | |||
|
|
|||
| def assertResponseHasErrors(self, resp, msg=None): | |||
| def assert_response_has_errors(self, resp, msg=None): | |||
There was a problem hiding this comment.
Same here, avoid breaking changes
There was a problem hiding this comment.
@sjdemartini thanks for the replies and for your very important suggestions - I'll take care of these soon enough.
Hello there!
Just wanna say I love this project so I went ahead and did some small contributions.
Pretty much just splitting out some functions that were too big, and fixed some typos.
While doing this I got paranoid with unit tests so I added a
tests-repeatcommand to theMakefilethat will allow you to run all unit tests, 100 times, in parallel - this can be useful in the future to catch bugs and/or flaky tests that can fail from time to time (which, after implementing, got some occasions where the tests failed but worked nicely after re-running - so maybe there are either undiscovered bugs and/or some flaky tests whose configuration - could be due to using some randomness on some tests - fail, well, randomly)The only function with a pretty high cognitive complexity that I didn't tackle (currently at 29) is the
graphene_django.types.DjangoObjectType.__init_subclass_with_meta__because it already looks pretty sequential and IMO not too hard to understand, just a lot of things that need to happen in it.Tested and formatted the code as per the contributing guidelines. Let me know if you have any questions and if you're ok with this contribution!