[AstroPy] Coding Guidelines draft (comments encouraged)

[Erik Tollerud]

Thanks for the careful reading - I'm compiling a list of suggested
changes to the guidelines themselves, and will update the document
itself once things are pretty much converged (example fixes you should
see now).

> "
> # The package should be importable with no dependencies other than the
> Python Standard Library (v2.6), NumPy, SciPy, matplotlib, and components
> already in the core Astronomy library."
>
> I was wondering which "Astronomy" library this refers to and what part
> is "core".
>
> I think it means "core Astropy", and I think later it defines "core
> Astropy" as any package that has already been accepted into astropy as
> astropy.whatever;  If this is correct, I recommend "in the core
> Astronomy library" -> "already in the Astropy package".

You're right, that was a typo from a version before we'd settled on
the astropy name - it's supposed to be "already in the Astropy core
package."  Thanks for catching that.


> I recommend that the template package include some small amount of C
> code and the Cython example for how to use it.

That makes a lot of sense, and should be quite straightforward.

> I note that the Cython recommendation implies an additional external
> dependency for somebody.  Is this for anybody who installs Astropy from
> source, or is it a preprocessor that you only run occasionally like
> yacc/lex?

It can be done either way, actually.  You can have Cython be a
requirement for the build process, in which case Cython builds the
C-code before compiling.  numpy/scipy mostly use an approach where
they always include the Cython source files, but also run Cython on
those files and include the generated .c files in the repository.  I'd
say our best approach would be to have developers use the first method
when they're actively working on the cython code, but whenever they've
reached a significant milestone, they generate and commit the .c file,
as well (and of course anytime there's a release).  I think this is
pretty much what numpy/scipy do, although I don't know if there's
actually a fixed policy or just sort of an understanding.


>    * If an external C library is needed, the source code for the
>      library should be bundled with the astropy core. Additionally, the
>      package must be compatible with using a system-installed library
>      instead of the version included in astropy."
>
>
> add "... using the mechanism shown in the template package."
>
> We want everybody to provide the same flags, same interface, etc, so we
> show one way to do it and ask everybody to do it that way.

Good point - will add this.

> If you want to require PEP 8, I suggest that you make your examples PEP
> 8 compliant.

Fair enough... we'll probably end up moving these in some form to the
example package where we'll hopefully be more careful, but setting a
good example even on the wiki page does make sense, so I've updated it
with pep8-checked version (as much as possible on wiki, anyway).

> I think we should explicitly recommend the new relative import mechanism
> any time you import another module/package that is in the same package
> you are part of.
>
> Say, for example, that you want to debug a package and you would like to
> run both the original and a modified copy at the same time.  With
> relative imports, you don't need to edit every import line in the whole
> package to make your modified copy.
>
> This is not compliant with PEP 8, which recommends the bad practice of
> always using absolute imports.  It made sense when relative imports were
> ambiguous, but the Python language no longer has that particular weakness.

Actually, there was a recent discussion of this by some of the python
committers ( http://mail.python.org/pipermail/python-committers/2011-February/001451.html
).  It looks like there's a variety of opinions even among them.  I
personally agree with you given the fact that relative imports now
actually work right... Does anyone else have a strong opinion on this?
 If not, I'll add an exception to PEP 8 along these lines.


> The property/get/set example is weak:  1) Take out the functions that
> are not relevant to get/set; you don't need them cluttering the
> example.  2) Show a user interacting with the object:
>
>     x = star()
>
>     # use this
>     x.color = 100
>     print x.color
>
>     # not this
>     x.set_color(100)
>     print x.get_color()

I see your point here... The intent was to show how the property is
different from direct attribute access - that's not obvious from the
example you're showing here.  But then again, when I think about it,
we will definitely have an example lick this in the example packages
to illustrate how this is implemented, anyway. So I've updated the
wiki with the simpler example.


> Realistically, I think people are likely to want to use the
> get_color()/set_color() approach because it is more natural.  It also
> makes it more clear what is happening.  I know that hard core OO guys
> think properties are cool, but I've never seen the attraction myself.

I don't find get_color/set_color to be natural at all - the
property/attribute access seems much more so to me... although I don't
think it's too hard to adapt from one style to the other (in either
direction) for this case.  The advantage of properties in this context
is that they encourage a natural flow between simple attribute access
(which should be used when nothing else is needed) to get/set methods
(properties look like attribute access).  On top of that, "star.color
= 0.4" is more readable, and is a bit faster to type than
"star.set_color(.4)".    Properties in the context given here are also
suggested by PEP8  (mostly for these reasons).

> The super() example actually shows more instances of doing it the wrong
> way than the right way, and it does not clearly mark which one to use.
>
> It should show one complete implementation using super() [marked "don't
> do this"] and another complete implementation using direct calls [marked
> "do this"].

The point of this example was to show what happens when you mix super
and direct calls, rather than to illustrate the correct way.  But I
see your point, so I've added a final code block for the direct call.


> One of the common cases for wanting to call into the superclass is to
> invoke the __init__() method.  The example should show how to do this,
> including the case of multiple inheritance.

This is now folded into the example, although it admittedly shows why
super really is sometimes useful if you're doing multiple inheritance
(i.e. the double-call to A.__init__).


> In the "import *" example, "__init__" is shown as "init" (two places).
> It looks like it needs a wiki escape.

Good catch - corrected.


-- 
Erik Tollerud