Fixes #29 #34
Fixes #29 #34
Conversation
|
hey @sramana, thanks for th PR! in general, it's important to give a descriptive heading and a helpful description of the PR you send, next time. Also, give a more descriptive commit messages. Thanks! |
There was a problem hiding this comment.
Please see my notes. Also try to keep close to the existing coding style, i.e. number of blank lines between stuff.
Also, It would be better if you could open this and future PR's against a separate branch, so that I can pull it easily and test/review locally.
Thanks a bunch!
| @@ -50,28 +50,31 @@ def get_object(self, factory): | |||
|
|
|||
| return factory.create() | |||
|
|
|||
| def setUp(self): | |||
| """Generates the main object and user instance if needed. | |||
| def get_user_factory(self): | |||
There was a problem hiding this comment.
This isn't a getter method, it actually sets user. If we're to follow the code style properly this should be a pure getter, simply for allowing developers override this method.
So this should simply getatter() on user_factory and return it.
| self.object = self.get_object(self.get_factory_class()) | ||
| def setUp(self): | ||
| """Generates the user instance if needed and the main object if required.""" | ||
| self.get_user_factory() |
There was a problem hiding this comment.
Just get back the user_factory and continue with generating user instance and authentication.
|
|
||
| # Avoid creating 2 objects if testing create. Use | ||
| # ```CreateAPITestCaseMixin.get_create_response()``` instead for ```test_create``` | ||
| if self._testMethodName != 'test_create': |
There was a problem hiding this comment.
This feels dirty. If I was to create a new test method that calls test_create I'd expect the same behavior, only that this would not work here.
I think adding a new flag would be more appropriate.
|
|
||
| The user instance will be created only if the ``user_factory`` attribute is set to the factory class. | ||
|
|
||
| If there is an available user instance, that user will be force authenticated. | ||
| """ | ||
|
|
There was a problem hiding this comment.
Please don't remove the blank lines, it's part of the code style.
| @@ -257,6 +262,9 @@ def get_lookup_from_response(self, data): | |||
| """ | |||
| return data.get(self.response_lookup_field) | |||
|
|
|||
| def get_model(self): | |||
| return self.model_class if hasattr(self, 'model_class') else self.object.__class__ | |||
There was a problem hiding this comment.
Using hasattr(self, 'model_class') isn't correct here since the attribute exists and is None. Try following the usage of getattr in the code.
Also, I think it would help the users if you add an assertion here that if the new flag for not creating the object is True AND model_class doesn't return a class then raise an exception with a proper explanation.
|
Hey @lukeaus, waiting your response on my review. Thanks! |
|
this project is dead, as far as I know |
|
Ping @lukeaus ; I ran into this. Can you finish this up? |
No description provided.