Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Australian Company Number validator #278

Merged
merged 4 commits into from
Mar 10, 2017
Merged

Add Australian Company Number validator #278

merged 4 commits into from
Mar 10, 2017

Conversation

koterpillar
Copy link
Contributor

@koterpillar koterpillar commented Feb 20, 2017

Add Australian Company Number model and form fields.

Note that running isort --recursive --line-width 120 localflavor tests produced changes in otherwise unchanged files. I've limited my changes to the files I touched.

All Changes

  • Add an entry to the docs/changelog.rst describing the change.

  • Add an entry for your name in the docs/authors.rst file if it's not
    already there.

  • Adjust your imports to a standard form by running this command:

    `isort --recursive --line-width 120 localflavor tests`
    

New Fields Only

  • Prefix the country code to all fields.

  • Field names should be easily understood by developers from the target
    localflavor country. This means that English translations are usually
    not the best name unless it's for something standard like postal code,
    tax / VAT ID etc.

  • Prefer 'PostalCodeField' for postal codes as it's
    international English; ZipCode is a term specific to the United
    States postal system.

  • Add meaningful tests. 100% test coverage is not required but all
    validation edge cases should be covered.

  • Add .. versionadded:: <next-version> comment markers to new
    localflavors.

  • Add documentation for all fields.

Copy link
Member

@benkonrath benkonrath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution and sorry for the delay in getting to this.

I fixed the issue with isort changing other files. Thanks for pointing that out.

The PR looks looks good. Just a couple of small changes and it's good to go.


def test_raises_error_for_whitespace(self):
"""Test an ACN can be valid when it contains whitespace."""
# NB: Form field should strip the whitespace before regex valdation is run.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valdation -> validation

I see that's also in the copy & pasted code so maybe you can also fix the typo there as well.

As this comment points out, the whitespace is stripped in the form. It would be nice to have a test for the form as well - at least testing the expected functionality that isn't tested with the validation tests.


validator = AUCompanyNumberFieldValidator()

validator(valid_acn)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to be consistent on the spacing. For instance, some tests have a space between each statement while one has no spaces. The space don't seem to be needed because it's pretty simple code. Feel free to fix the previous tests as well where this was copied from.

@koterpillar
Copy link
Contributor Author

Fixed both issues.

@benkonrath
Copy link
Member

It would be great if you can also add a test the form's prepare_value. I think there's an example test with the AUBusinessNumberField. Thanks.

@koterpillar
Copy link
Contributor Author

Added tests.

@benkonrath benkonrath merged commit 76c1a0e into django:master Mar 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants