Add support for Australian Business Numbers #63
Conversation
| invalid = { | ||
| '53004085617': error_format, | ||
| } | ||
| self.assertFieldOutput(AUBusinessNumberField, valid, invalid) |
There was a problem hiding this comment.
Tests are missing for the AUBusinessNumberField model field.
|
Added a few more tests. There are generally no tests for the models themselves except via the modelform. |
| class AUBusinessNumberField(CharField): | ||
| """ | ||
| A form field that validates input as an Australian Business Number (ABN) | ||
| """ |
There was a problem hiding this comment.
Can you add .. versionadded:: 1.1 here?
|
@danni are you still interested in this patch? The tests failed for the last update of your pull request. |
|
Definitely still interested. Hadn't realised Travis had failed. Wonder why it suddenly started failing on a docstring change... will investigate. |
|
Aah, it failed on the merge to master and test. Fix coming. |
|
@erikr: ping? |
|
There are a lot of one-char meaningless variable names used in the validation, which makes it unnecessarily hard to read. I'm not sure about the model field tests. The tests definitely cover the model field as well, but there are indeed no specific tests for it. Perhaps @jezdez has a particular opinion on this? |
|
Yeah, I think the one character stuff in the validator is unfortunate, @danni do you think you could fix those? With regard to model testing, I think it would make sense to have one, just to be sure the validation etc is picked up. |
|
I'd like to see this merged. Is it okay to rebase onto master and push to this PR, or should I create a new PR? |
|
@jscn go for it |
|
erikr and jezdez haven't spent much time on this project recently. I can take over this PR review but I'm a little pressed for time right now. I might able to spend some time next week. |
|
No worries, thanks for the update. |
|
A quick update: I'm at DjangoCon EU and plan to look at this during the sprints on Saturday and Sunday (if I don't find time before then). |
| """ | ||
| Return a cleaned ABN. | ||
| """ | ||
| value = super(AUBusinessNumberField, self).clean(value)\ |
There was a problem hiding this comment.
There should be a space before the ''.
You don't actually need to put the '.replace(...)' on a separate line because we're using 120 character lines in this project. That said, we're not enforcing a specific style yet so you can keep the shorter lines if that's your preference.
| try: | ||
| validator(valid_abn) | ||
| except ValidationError: | ||
| self.fail('Validation unexpectedly failed.') |
There was a problem hiding this comment.
Again, you don't need the try / except.
|
@jscn Sorry for the delay in reviewing this. As I mentioned, I wanted to do this during the sprints at DjangoCon EU but there were more people at the sprints that wanted to help out with localflavor than I expected. I've added my comments now and I'm looking forward to your thoughts. Thanks. |
|
@benkonrath I think I've fixed all the problems you identified, now. I'd really like to get this in for the 1.3 release, so let me know as soon as you can if there are any other changes you'd like to see. |
|
:/ apparently |
|
@jscn I'm just back online from a short vacation. Thanks for reworking this - it looks good. Just a couple of housekeeping items left:
I'd like to get this into 1.3 as well - I can delay by a day or two if you need a bit of time to sort this out. Thanks again for your work on this feature. |
|
@benkonrath Thanks for your help getting this in shape, I think it's all ready to go now, but let me know if there are further changes needed. |
|
Thank you too. :) |
No description provided.