Skip to content
This repository has been archived by the owner on Sep 18, 2024. It is now read-only.

Code cleanup. #26

Open
rlr opened this issue May 17, 2013 · 7 comments
Open

Code cleanup. #26

rlr opened this issue May 17, 2013 · 7 comments
Milestone

Comments

@rlr
Copy link
Contributor

rlr commented May 17, 2013

I was looking at the code and wasn't sure if this was python or java or javascript or something else. Joking aside, it doesn't follow the normal python coding styles.

In webdev at Mozilla, we are pretty strict following pep8 and such. See:
http://mozweb.readthedocs.org/en/latest/coding.html#python

Some things that should be fixed:

  • Use underscore_in_your_vars instead of camelCase. The classes are fine as CamelCase. This might be tricky now with the django models since it would affect column names. We can keep the column names the same using the db_column parameter or we could migrate everything to new database column names.
  • Use consistent 4-space indentation everywhere
  • 80 column limit per line
  • Add more docstrings
  • Make imports in the right order and group them
  • Be more consistent with new lines (two blank lines between the import statements and other code, two blank lines between each top level function/class)
  • Fix spaces around operators in some cases, such as form=RegistrationForm(instance = event) which should be form = RegistrationForm(instance=event)

Anyway, I volunteer to clean it up if nobody is opposed? It would create a huge merge conflict with any work-in-progress though.

Thoughts? Should I jump on this? It would make me more likely to contribute further to the project ❤️

@rlr
Copy link
Contributor Author

rlr commented May 17, 2013

Here is a tool that helps with this:
https://pypi.python.org/pypi/pep8

NOTE: this tool complains about some crazy things I don't care about.

@nukeador
Copy link
Member

Not a priority for now, we have too many issues pending to care about this. In fact, we don't really have a coding guide to follow at MH.

Probably this is a good discussion to bring to our labs list first.

@rlr
Copy link
Contributor Author

rlr commented May 17, 2013

Fine, I volunteered to do it and I could do it quickly (today). I'll post to the Labs list.

@rlr
Copy link
Contributor Author

rlr commented May 17, 2013

Also:

  • should use @property decorator instead of registrationOpen = property(registrationOpen)

@rlr
Copy link
Contributor Author

rlr commented May 20, 2013

Looks like @nukeador already went through and fixed Make imports in the right order and group them 👍

@nukeador
Copy link
Member

I think I took care of most imports, in any case, I'll try to check if they are OK each time I make a change to a .py file.

@rlr
Copy link
Contributor Author

rlr commented Jul 4, 2013

Did some cleanup 823b72f and e7351d0

More to come.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants