Skip to content

Conversation

@jnrbsn
Copy link
Contributor

@jnrbsn jnrbsn commented Apr 14, 2014

Please don't merge yet!

This pull request is to add the Python dev guidelines that I wrote. I've also moved everything into subdirectories for each language.

@Wikia/platform-team, @relwell, @tristaneuan: Please review this as well as the pylintrc file I wrote.

Click here to see the rendered version of the markdown, which is easier to read.

I want to eventually write some documentation on the recommended way to organize projects, setup virtual environments, deploy projects, etc. @nmonterroso and I are working out the details for that using the job queue project as a test.

@relwell
Copy link

relwell commented Apr 15, 2014

Couple quick things:

Single-Letter Variables

I think all single-letter variables should be allowed. Restricting to i and j is a little arbitrary (remember the zero/one/infinity rule!). PyCharm will complain if you're forced to reuse these "anonymous" variables within the same outer block. For instance, you've got an iteration over i, with a nested iteration over j, and then you reuse one of those in a later lambda function (e.g. lambda j: len(j)), you'll get squiggly lines.

While not explicitly defined in I'm pretty sure the standard practice is that numeric variables start at i and increment through the alphabet, while lambda arguments start at x and increment. Rather than create two restrictive subsets, I think it just makes sense to leave this one to common sense and collaboration, since there's an appropriate place and time for it in pythonic code.

Sphinx Documentation Format

I'd like to encourage that we go one step further than PEP257 and to use the Sphinx documentation format as our standard documentation format. There's a ton of advantages to this:

  • Explicit type-hinting that gets implemented in your IDE (i.e. prediction of return value types, warnings that an exception that could be thrown isn't being handled, etc.)
  • The ability to auto-generate super-thorough documentation (e.g. readthedocs for DeployTools!)
  • Consistent expectations across our PHP and Python code that we provide thorough, structured documentation on the types of parameters that are passed to a given function

That's all I've got. Thanks for reading!

@t33chong
Copy link

+1 on Sphinx docstrings.

I'm not too keen on the 120-character line length, though. I work with a bunch of Vim splits and I value screen real estate. Adjusting from the standard 80 characters would cut down the number of panes I can have open by a third. If you follow PEP-8, there should be very few scenarios in which lines longer than 80 characters are unavoidable.

What's the advantage of pylint over flake8?

@relwell
Copy link

relwell commented Apr 15, 2014

Gonna have to give a 👎 on reducing the string length. That's
about 10% of horizontal space being taken up by white space if you're
writing logic for a method inside a class definition. I've got enough
trouble keeping it to 120 characters as it is.

On Tue, Apr 15, 2014 at 10:55 AM, tristaneuan notifications@github.comwrote:

+1 on Sphinx docstrings.

I'm not too keen on the 120-character line length, though. I work with a
bunch of Vim splits and I value screen real estate. Adjusting from the
standard 80 characters would cut down the number of panes I can have open
by a third. If you follow PEP-8, there should be very few scenarios in
which lines longer than 80 characters are unavoidable.

What's the advantage of pylint over flake8?

Reply to this email directly or view it on GitHubhttps://github.com//pull/17#issuecomment-40513074
.

@jnrbsn
Copy link
Contributor Author

jnrbsn commented Apr 15, 2014

@relwell & @tristaneuan:

Single-letter variables: I'm fine with allowing single-letter variable names, but I would like to add a warning in the document about their overuse. I definitely think they're great for iteration and for lambdas, but I hate reading code that has a ton of ambiguously named variables.

Sphinx docstrings: I agree about the Sphinx docstrings. I kind of wanted to be a little lenient about docstrings, because I feel like that's often the type of thing people find most annoying about coding conventions. I think for now, I'd like to make it simply a recommendation, and not imply that it's a requirement. Are you guys fine with that? You can still pick on people when they don't use it. 😉

80 vs. 120 character line length: I have to agree with Robert on this. Sorry. Feel free to write your own code to be 80 characters or less, but you're going to have a hard time convincing other people to do it. Let's see what everyone else says. By the way, 120 is the limit we're using for other languages, and it's also pretty much the perfect width for browsing code on GitHub.

@relwell
Copy link

relwell commented Apr 15, 2014

I think not requiring sphinx docstrings makes sense. Explicitly
recommending them in the guidelines is a good way to encourage people to
use them.

On Tue, Apr 15, 2014 at 11:10 AM, Jonathan Robson
notifications@github.comwrote:

@relwell https://github.com/relwell & @tristaneuanhttps://github.com/tristaneuan
:

Single-letter variables: I'm fine with allowing single-letter variable
names, but I would like to add a warning in the document about their
overuse. I definitely think they're great for iteration and for lambdas,
but I hate reading code that has a ton of ambiguously named variables.

Sphinx docstrings: I agree about the Sphinx docstrings. I kind of
wanted to be a little lenient about docstrings, because I feel like that's
often the type of thing people find most annoying about coding conventions.
I think for now, I'd like to make it simply a recommendation, and not imply
that it's a requirement. Are you guys fine with that? You can still pick on
people when they don't use it. [image: 😉]

80 vs. 120 character line length: I have to agree with Robert on this.
Sorry. Feel free to write your own code to be 80 characters or less, but
you're going to have a hard time convincing other people to do it. Let's
see what everyone else says. By the way, 120 is the limit we're using for
other languages, and it's also pretty much the perfect width for browsing
code on GitHub.

Reply to this email directly or view it on GitHubhttps://github.com//pull/17#issuecomment-40514710
.

@jnrbsn
Copy link
Contributor Author

jnrbsn commented Apr 15, 2014

@tristaneuan (forgot to answer your last question):

The reason we chose pylint is simply because it seemed like the most popular tool of its type. I'll look at flake8 and see if it provides anything that we would want that pylint doesn't already do. At some point, I'll probably create a module that wraps our various code checkers and automatically uses our settings to make it easier for people to use.

@t33chong
Copy link

Before
After

If you're not convinced, that's fine, but it is possible.

@relwell
Copy link

relwell commented Apr 15, 2014

Now take some screen shots in PyCharm ;)

On Tue, Apr 15, 2014 at 11:15 AM, tristaneuan notifications@github.comwrote:

Before:
[image: screen shot 2014-04-15 at 11 09 54 am]https://cloud.githubusercontent.com/assets/4148627/2711081/c4da6d48-c4c9-11e3-8c5a-762050e5cccc.png
After:
[image: screen shot 2014-04-15 at 11 12 31 am]https://cloud.githubusercontent.com/assets/4148627/2711084/cbd8bad2-c4c9-11e3-8c4d-1181f39ca47e.png

If you're not convinced, that's fine, but it is possible.

Reply to this email directly or view it on GitHubhttps://github.com//pull/17#issuecomment-40515305
.

@jnrbsn
Copy link
Contributor Author

jnrbsn commented Apr 15, 2014

This conversation is going to become a scrolling nightmare if we post a bunch of screenshots.

@tristaneuan, you're not going to convince me. Sorry. Let's just wait and see what everyone else says. Remember, these are just guidelines for all of Wikia. If you want to require 80 character line lengths in a project, then you can work it out with the people involved in just that project.

@kenkouot
Copy link
Contributor

+1 for 80 column line lengths. I know our JS style guidelines are at 120, but personally, I find 80 much more readable.

@nmonterroso
Copy link
Contributor

Since it's a tie, -1 for 80 column line lengths. It's 2014, guys.

@gabrys
Copy link

gabrys commented Apr 15, 2014

2014 is a hell long line

@kenkouot
Copy link
Contributor

@gabrys 👅

@themech
Copy link

themech commented Apr 17, 2014

Looks ok, great work. I'm also for allowing single-letter var names for counters and iterators, this is a common practice in Python. I also like prepending protected and private method names with single/double underscores, but that's already covered by Google guidelines.
And yeah, 👍 for 120 column line lenghts :)

@federico-lox
Copy link
Contributor

Good job.

👍 for 120 column line lengths.

What the thread thinks about adding the following?

  • Apps should always include a requirements.txt in their repos with all the necessary dependencies declared
    • There's nothing more time consuming and annoying than the run-fail-install loop 👿
    • Recently some projects have introduced a requirements-dev.txt to split out non-production utils/packages, e.g. stuff for testing and profiling; I think is a nice way to keep production servers uncluttered
  • Packages and apps to be published through local/public pip repos should include a setup.py file
    • this should be strictly required especially for anything needing custom CPython native extensions to be compiled and installed

@jnrbsn
Copy link
Contributor Author

jnrbsn commented Apr 17, 2014

@federico-lox: I don't want to include that info right now. As I said in the PR description...

I want to eventually write some documentation on the recommended way to organize projects, setup virtual environments, deploy projects, etc. @nmonterroso and I are working out the details for that using the job queue project as a test.

There's still some debate about the best way to organize projects. Robert, Nelson, and I have been discussing this and doing related research. I don't want to have that conversation here though. This is just coding conventions for now. The other stuff will come soon after this.

@federico-lox
Copy link
Contributor

@jnrbsn gotcha 👍

@jnrbsn
Copy link
Contributor Author

jnrbsn commented Apr 18, 2014

Ok. I've made the changes agreed upon in the conversation here. I'm going to merge it now.

jnrbsn added a commit that referenced this pull request Apr 18, 2014
@jnrbsn jnrbsn merged commit fedb73a into master Apr 18, 2014
@jnrbsn jnrbsn deleted the jrobson-python-guidelines branch April 18, 2014 18:09
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.

8 participants