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

issue #214 fixed pep8, pep257 and other styling errors to pass the pr… #259

Merged
merged 1 commit into from
Nov 6, 2016

Conversation

unloder
Copy link
Contributor

@unloder unloder commented Nov 6, 2016

I have fixed most pep8, pep257 and other styling errors that didn't let prospector validate
Added a custom config for prospector, that inherits hygh_strictness, but has a line length setting changed to 120, and has pep8 N802 disabled because it is conflicting with test naming.

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`
    

@unloder unloder force-pushed the prospector-fixes branch 2 times, most recently from 511d883 to 140f73a Compare November 6, 2016 11:41
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.

Looks good. Just a few changes before this can be merged.

A model field that is represented with
:data:`~localflavor.au.au_states.STATE_CHOICES`` choices and
stores the three-letter Australian state abbreviation in the database.
A model field stores the three-letter Australian state abbreviation in the database.
Copy link
Member

Choose a reason for hiding this comment

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

This should be: "A model field that stores ..."

if value not in valid_values:
raise ValidationError(self.error_messages['invalid'])
return value


def DV_maker(v):
def dv_maker(v):
Copy link
Member

Choose a reason for hiding this comment

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

This public method needs to be deprecated before it's changed. Can you add a note about this to #258?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a DV_maker method that uses dv_maker for backwards compatibility

raise ValidationError(self.error_messages['invalid'])

if value[:6] == '000000' or value[12:] == '0000':
raise ValidationError(self.error_messages['invalid'])

return '%s.%s.%s.%s' % (value[:2], value[2:6], value[6:12], value[12:])

@staticmethod
def _valid_nik_date(year, month, day):
Copy link
Member

Choose a reason for hiding this comment

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

Again this is a public method that's being changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

def valid_nik_date(year, month, day): is not a public method, it is a local function inside the clean method so it is not exposed publicly

stores the five-letter Pakistani state abbreviation in the database.
A model field that stores the five-letter Pakistani state abbreviation in the database.

It is represented with :data:`~localflavor.pk.pk_states.STATE_CHOICES`` choices and.
Copy link
Member

Choose a reason for hiding this comment

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

The extra 'and' at the end should be removed.

* Fixed pep8, pep257 and other styling errors to pass the prospector validation.
* Added custom prospector config that skips pep8 n802 checks
	(because the naming in tests is not compliant, but is conventional).
* Some of the errors have been hidden by # noqa comments and added as a separate issue django#258.
* Turned on prospector in tox.ini.
fixes django#214
@benkonrath
Copy link
Member

Looks good. Thanks!

@benkonrath benkonrath merged commit 2fd51e9 into django:master Nov 6, 2016
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.

None yet

2 participants