Hi Chris, On Fri, Jul 12, 2013 at 7:45 PM, Christopher Medrela < [email protected]> wrote:
> Finally, I've rewritten all model and field tests and checks. Some new > tests > were written. First draft of checking framework documentation was written. > All > tests passes (at least at my computer). I've rebased my branch > (`gsoc2013-checks` [1]) against master. The branch is ready to deep review. > > [1] https://github.com/chrismedrela/django/tree/gsoc2013-checks > This is looking good! If you issue a pull request for this branch, I'll add some detailed comments there. When you make the pull request, make sure you add a comment noting that this is a work in progress associated with the GSoC, and isn't a candidate for merging (yet!). > I've created pull request [2] for ticket [#19934]. The patch clarifies that > importing `django.utils.image` cannot result in ImportError. I've also > created > ticket [#20735] about confusing ManyToManyField documentation. There is a > pull > request too [3]. Both pull requests were merged. > > [2] https://github.com/django/django/pull/1349 > [3] https://github.com/django/django/pull/1350 > [#19934] https://code.djangoproject.com/ticket/19934 > [#20735] https://code.djangoproject.com/ticket/20735 > 1. I wrote some tests [4] where nose tests generators [5] would be really > helpful. I tried to emulate them. Is the code I wrote a good approach? > > [4] > https://github.com/chrismedrela/django/blob/1c14c674aa52c666ec8742c72657d5dad2efe0cb/tests/invalid_models/tests.py#L690 > [5] > https://nose.readthedocs.org/en/latest/writing_tests.html#test-generators > I can see what you're doing here, but it's probably not the best approach. As implemented, there are two problems: 1) there's going to be a redundant call to setUp and tearDown 2) you only get one opportunity for a failure in all the accessor_check tests. Given that you're only dealing with 6 tests (2 targets x 3 relatives) here, I'd be inclined to manually roll them out. When we've got Python 3.4 as a minimum dependency, we can start using subunit tests [1], but until then, we can live with a small amount of manual code rollout. [1] http://docs.python.org/dev/library/unittest.html#distinguishing-test-iterations-using-subtests > 2. Two m2m through the same model are forbidden. The new error message is: > > applabel.modelname: Two m2m relations through the same model. > The model has two many-to-many relations through the intermediary > Membership > model, which is not permitted. > (No hint.) > > > Is there any reasonable hint we can provide except for suggesting > duplicating > the intermediary model? > The hint is optional, so there's not much point having a hint unless it's actually providing useful information that isn't implicit in the error message. You can't have 2 m2m relations through the same intermediate model. The fix - don't have 2 m2m relations through the same intermediate model. The only practical advice we could give here would be to duplicate the intermediate model, but that doesn't strike me as something that is likely to be correct advice under most circumstances, so I'm hesitant to provide it as default advice. > 3. I've deleted `app_errors` [6] because it wasn't used anywhere (inside > Django). Was it left by accident or was there a reason for that? > > [6] > https://github.com/chrismedrela/django/commit/3b221caefc151f7750a322e18798c22343daba6f > I'm not aware of anywhere that it is being used; I'm guessing it exists for historical reasons, but I have no idea what those are. If the tests still pass when you've removed this code, I see no reason to keep it around. > 4. I've renamed DatabaseValidation.validate_field to check_field and > changed its > arguments. They were: `errors` -- ModelErrorCollection [7], `opts` -- meta > of > the model of the field and `f` -- the field; The method returned None. Now > there > is only `field` argument and the method returns list of errors. It's not > public > API, but we should go through deprecation process. > > How can we do that regarding changed arguments? We can call old > `validate_field` > method where the backend specific validation is triggered [8]. By default, > the > old method will call `check_field` method. The old one need to transform > list of > errors into strings and add them to `errors` list. The problem is that at > [8] we > need to convert the strings again to errors. That cannot be done in a > graceful > way. Is there any other way to achieve deprecation? > > I'd suggest that we ship both APIs -- validate_field() unmodified to support older codebases (but internally documented to indicate that we're moving away from that approach), and a new check_field() method that does things the New Way. As a migration aid, the default implementation of check_field() could invoke validate_field() and just convert the output into the new format -- it should be possible to retrieve all the required arguments from the data you already have available, so it's just a matter of taking the 'list of error strings' and converting it into a 'List of Error objects'. If a third party developer wants to provide richer support (including warnings, hints etc) then they can write a check_field() implementation in parallel to the existing validate_fields() call. Yours, Russ Magee %-) -- You received this message because you are subscribed to the Google Groups "Django developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/django-developers. For more options, visit https://groups.google.com/groups/opt_out.
