Style Guide #257

Merged
merged 13 commits into from Feb 2, 2014

Conversation

Projects
None yet
7 participants
@scopatz
Member

scopatz commented Jan 17, 2014

No description provided.

@@ -3,8 +3,8 @@
///
/// \brief Top-level enrichment functionality.
-#if !defined(_PYNE_ENRICHMENT_)
-#define _PYNE_ENRICHMENT_
+#if !defined(_4fd11fd5_2aa1_4dd1_9561_d8af45cab5ef)

This comment has been minimized.

@gonuke

gonuke Jan 17, 2014

Contributor

Is this intentional? UUIDs (?) as include guards?

@gonuke

gonuke Jan 17, 2014

Contributor

Is this intentional? UUIDs (?) as include guards?

This comment has been minimized.

@scopatz

scopatz Jan 18, 2014

Member

Yes. This was inspired by John Bachan. See this post. I think it is the right way to do include guards. Though if you disagree we should have a discussion about it on the PyNE list.

@scopatz

scopatz Jan 18, 2014

Member

Yes. This was inspired by John Bachan. See this post. I think it is the right way to do include guards. Though if you disagree we should have a discussion about it on the PyNE list.

This comment has been minimized.

@crbates

crbates Jan 18, 2014

Contributor

A uuid seems excessively long. I sorta like the idea of base64 encoding it (or just using the first 32 bits) but that is mainly an aesthetic thing. Something like:

python -c "import uuid; print uuid.uuid4().bytes.encode('base64').rstrip('=\n').replace('/', '_').replace('+','_')"

that generates shorter unique id's like:
mrjuSg0qRhCOcgaqstMOKw

@crbates

crbates Jan 18, 2014

Contributor

A uuid seems excessively long. I sorta like the idea of base64 encoding it (or just using the first 32 bits) but that is mainly an aesthetic thing. Something like:

python -c "import uuid; print uuid.uuid4().bytes.encode('base64').rstrip('=\n').replace('/', '_').replace('+','_')"

that generates shorter unique id's like:
mrjuSg0qRhCOcgaqstMOKw

This comment has been minimized.

@scopatz

scopatz Jan 18, 2014

Member

Oooo. I like it! Here is a version I like a little more:

$ python -c "import uuid; import base64; print('_' + base64.b32encode(uuid.uuid4().bytes).decode().strip('='))"
_FEMRL3LRUJFQRNHBXY444HDEGU

The advantage here is that it follows the all uppercase macro style, has a leading underscore to prevent strings starting in 0 - 9 from being invalid identifiers, and works on both Python 2 and Python 3.

@scopatz

scopatz Jan 18, 2014

Member

Oooo. I like it! Here is a version I like a little more:

$ python -c "import uuid; import base64; print('_' + base64.b32encode(uuid.uuid4().bytes).decode().strip('='))"
_FEMRL3LRUJFQRNHBXY444HDEGU

The advantage here is that it follows the all uppercase macro style, has a leading underscore to prevent strings starting in 0 - 9 from being invalid identifiers, and works on both Python 2 and Python 3.

This comment has been minimized.

@crbates

crbates Jan 19, 2014

Contributor

Nice! I guess It's really only 10 characters shorter but it feels more compact.

@crbates

crbates Jan 19, 2014

Contributor

Nice! I guess It's really only 10 characters shorter but it feels more compact.

docs/devsguide/style_guide.rst
+* Use 'single quotes' for string literals, and """triple double quotes""" for
+ docstrings. Double quotes are allowed to prevent single quote escaping,
+ e.g. "Y'all c'mon o'er here!"
+

This comment has been minimized.

@crbates

crbates Jan 20, 2014

Contributor

I think we should add using print_function and division from __future__ where applicable in both python and cython.

@crbates

crbates Jan 20, 2014

Contributor

I think we should add using print_function and division from __future__ where applicable in both python and cython.

This comment has been minimized.

@scopatz

scopatz Jan 20, 2014

Member

Defintiley. I actually think that we should require the python to be written in a Python 2/3 single code base style.... until such a time that Python 2 is quite dead. What say you?

@scopatz

scopatz Jan 20, 2014

Member

Defintiley. I actually think that we should require the python to be written in a Python 2/3 single code base style.... until such a time that Python 2 is quite dead. What say you?

This comment has been minimized.

@crbates

crbates Jan 20, 2014

Contributor

I totally agree. We should also specify our specific line length preference here as PEP8 recommends but does not require 79 characters for code and 72 code docs/comments. I don't mind sticking with 80 across the board just for consistency, but I think we should mention it here. relevant PEP section: http://www.python.org/dev/peps/pep-0008/#maximum-line-length

@crbates

crbates Jan 20, 2014

Contributor

I totally agree. We should also specify our specific line length preference here as PEP8 recommends but does not require 79 characters for code and 72 code docs/comments. I don't mind sticking with 80 across the board just for consistency, but I think we should mention it here. relevant PEP section: http://www.python.org/dev/peps/pep-0008/#maximum-line-length

This comment has been minimized.

@scopatz

scopatz Jan 20, 2014

Member

Good point. I like 80 across the board.

@scopatz

scopatz Jan 20, 2014

Member

Good point. I like 80 across the board.

docs/devsguide/style_guide.rst
+
+ # Bad
+ cdef char *s
+ cpdef int i, j, k

This comment has been minimized.

@crbates

crbates Jan 20, 2014

Contributor

Whats the thinking of one variable per line? Just that it's more pythonic and legible?

@crbates

crbates Jan 20, 2014

Contributor

Whats the thinking of one variable per line? Just that it's more pythonic and legible?

This comment has been minimized.

@scopatz

scopatz Jan 20, 2014

Member

I went back and forth on this. Mostly it is legibility even though it eats up precious vertical space. I actually think that just declaring simple variables is OK. Where it starts to be illegible are cases like this:

cdef char * s, * t, * u, * v
cdef double x=42, y=x+1, z=x*y 

If you can think of a way to consistently disclude these but include the simple cdef int i, j, k, maybe that should go in.

@scopatz

scopatz Jan 20, 2014

Member

I went back and forth on this. Mostly it is legibility even though it eats up precious vertical space. I actually think that just declaring simple variables is OK. Where it starts to be illegible are cases like this:

cdef char * s, * t, * u, * v
cdef double x=42, y=x+1, z=x*y 

If you can think of a way to consistently disclude these but include the simple cdef int i, j, k, maybe that should go in.

+All code must be documented. We use doxygen to auto-document our code.
+All functions, methods, classes, variables, enumerations, and constants
+should be documented.
+

This comment has been minimized.

@crbates

crbates Jan 20, 2014

Contributor

Do we use doxygen for c++? I thought it was all sphinx.

@crbates

crbates Jan 20, 2014

Contributor

Do we use doxygen for c++? I thought it was all sphinx.

This comment has been minimized.

@scopatz

scopatz Jan 20, 2014

Member

We are sneaky here. We use breathe to expose doxygen output automatically to sphinx. So for purposes of the style guide the C/C++ docstrings should be in doxygen format.

@scopatz

scopatz Jan 20, 2014

Member

We are sneaky here. We use breathe to expose doxygen output automatically to sphinx. So for purposes of the style guide the C/C++ docstrings should be in doxygen format.

@scopatz

This comment has been minimized.

Show comment
Hide comment
@scopatz

scopatz Jan 20, 2014

Member

Updated style guide.

Member

scopatz commented Jan 20, 2014

Updated style guide.

@erelson

This comment has been minimized.

Show comment
Hide comment
@erelson

erelson Jan 21, 2014

Member

Should notes regarding tests be included? The only one I have in mind at the moment is avoiding test classes derived from UnitTest (iirc), which came up a month or so ago (I think with the ssw additions to mcnp.py). This might not quite qualify as style, though.

Member

erelson commented Jan 21, 2014

Should notes regarding tests be included? The only one I have in mind at the moment is avoiding test classes derived from UnitTest (iirc), which came up a month or so ago (I think with the ssw additions to mcnp.py). This might not quite qualify as style, though.

@scopatz

This comment has been minimized.

Show comment
Hide comment
@scopatz

scopatz Jan 21, 2014

Member

This qualifies @erelson. Thanks. I'll update.

Member

scopatz commented Jan 21, 2014

This qualifies @erelson. Thanks. I'll update.

@scopatz

This comment has been minimized.

Show comment
Hide comment
@scopatz

scopatz Jan 21, 2014

Member

Updated.

Member

scopatz commented Jan 21, 2014

Updated.

-#if !defined(_PYNE_ENRICHMENT_)
-#define _PYNE_ENRICHMENT_
+#if !defined(_4fd11fd5_2aa1_4dd1_9561_d8af45cab5ef)
+#define _4fd11fd5_2aa1_4dd1_9561_d8af45cab5ef

This comment has been minimized.

@crbates

crbates Jan 23, 2014

Contributor

@scopatz These guards should be updated to the new shorter style. Then I think this is ready to merge.

@crbates

crbates Jan 23, 2014

Contributor

@scopatz These guards should be updated to the new shorter style. Then I think this is ready to merge.

This comment has been minimized.

@scopatz

scopatz Jan 23, 2014

Member

Yup, they need to be. And actually, this file snuck into the commit accidentally. I expect that once we accept the style guide we won't be in compliance with it. I think that is a separate, later issue.

However, I want to hold off on merging in the SG until we hear back from some more people. To call them out, @elliottbiondo, @gidden, @paulromano, @gonuke, @kmanalo, @katyhuff, and anyone else who has or may seriously work on the code base. I don't mind letting this PR sit as long as people are reviewing it. I think that it will be harder to change later on than it is to accept initially so I don't want anyone to feel like they didn't get their fair chance to voice their concerns.

@scopatz

scopatz Jan 23, 2014

Member

Yup, they need to be. And actually, this file snuck into the commit accidentally. I expect that once we accept the style guide we won't be in compliance with it. I think that is a separate, later issue.

However, I want to hold off on merging in the SG until we hear back from some more people. To call them out, @elliottbiondo, @gidden, @paulromano, @gonuke, @kmanalo, @katyhuff, and anyone else who has or may seriously work on the code base. I don't mind letting this PR sit as long as people are reviewing it. I think that it will be harder to change later on than it is to accept initially so I don't want anyone to feel like they didn't get their fair chance to voice their concerns.

+
+The contents of a namespace are not indented.
+
+We are all friends here! Braces should be `cuddled <http://gskinner.com/blog/archives/2008/11/curly_braces_to.html>`_:

This comment has been minimized.

@katyhuff

katyhuff Jan 23, 2014

Member

Adorable. I love this.

@katyhuff

katyhuff Jan 23, 2014

Member

Adorable. I love this.

This comment has been minimized.

@scopatz

scopatz Jan 23, 2014

Member

:)

docs/devsguide/style_guide.rst
+ ...
+ }
+
+Braces should be omitted if the enclosed block is a single-line statement:

This comment has been minimized.

@katyhuff

katyhuff Jan 23, 2014

Member

Do you have a motivating reason for this choice? I imagine you do. Does this come from a particular style guide?

I personally usually prefer to have braces always. The reasoning, for me, is : If they're present in the 1-line statement, then, when you add another line to that statement down the road, you won't forget to add the braces... because they'll already be there.

@katyhuff

katyhuff Jan 23, 2014

Member

Do you have a motivating reason for this choice? I imagine you do. Does this come from a particular style guide?

I personally usually prefer to have braces always. The reasoning, for me, is : If they're present in the 1-line statement, then, when you add another line to that statement down the road, you won't forget to add the braces... because they'll already be there.

This comment has been minimized.

@gidden

gidden Jan 23, 2014

Contributor

I'm also curious why the style guide is more restrictive than the gscg here. More uniformity?

@gidden

gidden Jan 23, 2014

Contributor

I'm also curious why the style guide is more restrictive than the gscg here. More uniformity?

This comment has been minimized.

@scopatz

scopatz Jan 23, 2014

Member

This is to preserve precious vertical space. It is a much bigger deal when braces are not cuddled and the signal to noise ratio is 1:1.

Also this only really bites when there is no else clause and the 1+n lines are executed irrespective of the condition. If there is an else clause, it is a normally a compile time error and this is caught very early.

I am happy to change this to "Braces may be omitted..." That reflects more how I feel anyway :)

@scopatz

scopatz Jan 23, 2014

Member

This is to preserve precious vertical space. It is a much bigger deal when braces are not cuddled and the signal to noise ratio is 1:1.

Also this only really bites when there is no else clause and the 1+n lines are executed irrespective of the condition. If there is an else clause, it is a normally a compile time error and this is caught very early.

I am happy to change this to "Braces may be omitted..." That reflects more how I feel anyway :)

+Inheritance
+***********
+When overriding a virtual method in a subclass always declare it to be virtual
+so that the reader knows what's going on.

This comment has been minimized.

@katyhuff

katyhuff Jan 23, 2014

Member

👍 I feel strongly about this.

@katyhuff

katyhuff Jan 23, 2014

Member

👍 I feel strongly about this.

docs/devsguide/style_guide.rst
+Exceptions
+**********
+Exceptions are the preferred error-reporting mechanism,
+as opposed to returning integer error codes.

This comment has been minimized.

@katyhuff

katyhuff Jan 23, 2014

Member

Is it worth stating explicitly that we strongly prefer built-in exceptions where possible?

@katyhuff

katyhuff Jan 23, 2014

Member

Is it worth stating explicitly that we strongly prefer built-in exceptions where possible?

This comment has been minimized.

@scopatz

scopatz Jan 23, 2014

Member

Yes. Definitely. I'll add that.

@scopatz

scopatz Jan 23, 2014

Member

Yes. Definitely. I'll add that.

+**************
+Only call ``exit()`` at a well-defined exit point for the application.
+
+Never call ``exit()`` in a library.

This comment has been minimized.

@katyhuff

katyhuff Jan 23, 2014

Member

Whoa! ACK! Do people do this? Is this a thing? How did you even think to protect against this?

Anyway, agreed.

@katyhuff

katyhuff Jan 23, 2014

Member

Whoa! ACK! Do people do this? Is this a thing? How did you even think to protect against this?

Anyway, agreed.

This comment has been minimized.

@crbates

crbates Jan 23, 2014

Contributor

I've seen libraries that do this instead of throwing exceptions. It's really annoying.

@crbates

crbates Jan 23, 2014

Contributor

I've seen libraries that do this instead of throwing exceptions. It's really annoying.

This comment has been minimized.

@scopatz

scopatz Jan 23, 2014

Member

If you ever feel the need to be truly maniacal, write a tool that randomly adds one exit() call in a code base for every 100k lines. Make sure this call is protected and isn't executed when DEBUG mode is on or while testing. Bury the change in a 1000+ line commit. Bonus points if you can find a function which is only called once with no arguments. Add the exit call as a define flag in the build system :).

@scopatz

scopatz Jan 23, 2014

Member

If you ever feel the need to be truly maniacal, write a tool that randomly adds one exit() call in a code base for every 100k lines. Make sure this call is protected and isn't executed when DEBUG mode is on or while testing. Bury the change in a 1000+ line commit. Bonus points if you can find a function which is only called once with no arguments. Add the exit call as a define flag in the build system :).

@katyhuff

This comment has been minimized.

Show comment
Hide comment
@katyhuff

katyhuff Jan 23, 2014

Member

I think this looks great! I've enjoyed the discussion so far. I am curious about the curly-braces-with-a-one-line-statement point, but I'm not militant about my opinion, so, I'd call this ready to pull.

Member

katyhuff commented Jan 23, 2014

I think this looks great! I've enjoyed the discussion so far. I am curious about the curly-braces-with-a-one-line-statement point, but I'm not militant about my opinion, so, I'd call this ready to pull.

+* The capitalized project name is "PyNE" while the lowercase project name is "pyne".
+* Write code in whatever language you want but make sure that it is exposed to Python.
+ Python is the glue that holds the universe together. It is the ``NULL`` and the
+ ``INT_MAX``.

This comment has been minimized.

@gidden

gidden Jan 23, 2014

Contributor

I lol'ed

@gidden

gidden Jan 23, 2014

Contributor

I lol'ed

+Canonical Forms
+***************
+* We have canonical forms; use them! For example, if a function accepts a nuclide
+ as an argument then you should use nucname to ensure that it accepts all possible

This comment has been minimized.

@gidden

gidden Jan 23, 2014

Contributor

nuc_name?

@gidden

gidden Jan 23, 2014

Contributor

nuc_name?

This comment has been minimized.

@scopatz

scopatz Jan 23, 2014

Member

nucname is the module name. nuc_name is my personal convention for the name of a specific nuclide in human readable form, "U-235". Not super happy with this convention so I didn't make it part of the style guide.

@scopatz

scopatz Jan 23, 2014

Member

nucname is the module name. nuc_name is my personal convention for the name of a specific nuclide in human readable form, "U-235". Not super happy with this convention so I didn't make it part of the style guide.

+ with geometries.
+* If a canonical form doesn't exist, follow these steps:
+
+ 1. invent one, and

This comment has been minimized.

@gidden

gidden Jan 23, 2014

Contributor

don't need to indent for first level lists in rst

@gidden

gidden Jan 23, 2014

Contributor

don't need to indent for first level lists in rst

This comment has been minimized.

@gidden

gidden Jan 23, 2014

Contributor

nevermind, makes sense here (sorry)

@gidden

gidden Jan 23, 2014

Contributor

nevermind, makes sense here (sorry)

This comment has been minimized.

@scopatz

scopatz Jan 23, 2014

Member

It is almost like we need some sort of automatic html rendering and diffing tool that is fully integrated with github pull requests....

@scopatz

scopatz Jan 23, 2014

Member

It is almost like we need some sort of automatic html rendering and diffing tool that is fully integrated with github pull requests....

docs/devsguide/style_guide.rst
+If the file primarily implements a class, name the file after the class.
+
+****************************
+Classes, Typedefs, & Stucts

This comment has been minimized.

@crbates

crbates Jan 23, 2014

Contributor

Stucts -> Structs

@crbates

crbates Jan 23, 2014

Contributor

Stucts -> Structs

This comment has been minimized.

@scopatz

scopatz Jan 23, 2014

Member

fixed.

@scopatz

scopatz Jan 23, 2014

Member

fixed.

@scopatz

This comment has been minimized.

Show comment
Hide comment
@scopatz

scopatz Jan 23, 2014

Member

More minor updates.

Member

scopatz commented Jan 23, 2014

More minor updates.

docs/devsguide/style_guide.rst
+
+ // Good
+ using std::list; // I want to refer to std::list as list
+ using std::vector; // I want to refer to std::vector as vector

This comment has been minimized.

@crbates

crbates Jan 23, 2014

Contributor

This is really minor but previously "good" code came first and "bad" code came second. Can we switch these statements for continuity.

@crbates

crbates Jan 23, 2014

Contributor

This is really minor but previously "good" code came first and "bad" code came second. Can we switch these statements for continuity.

This comment has been minimized.

@scopatz

scopatz Jan 23, 2014

Member

Yes. This is a good point about my bad style.

@scopatz

scopatz Jan 23, 2014

Member

Yes. This is a good point about my bad style.

@erelson

This comment has been minimized.

Show comment
Hide comment
@erelson

erelson Jan 26, 2014

Member

We should definitely note (and link to) the numpydoc format that we use for (python) docstrings.

E.g. https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt#method-docstrings

Member

erelson commented Jan 26, 2014

We should definitely note (and link to) the numpydoc format that we use for (python) docstrings.

E.g. https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt#method-docstrings

@scopatz

This comment has been minimized.

Show comment
Hide comment
@scopatz

scopatz Jan 26, 2014

Member

Thanks for pointing this out @erelson. done.

Member

scopatz commented Jan 26, 2014

Thanks for pointing this out @erelson. done.

docs/devsguide/style_guide.rst
+
+-------------------
+C/C++ Style Guide
+-------------------

This comment has been minimized.

@crbates

crbates Jan 26, 2014

Contributor

Based on our discussion in #271 should we mention C++03 as the de-facto standard for c++ code.

@crbates

crbates Jan 26, 2014

Contributor

Based on our discussion in #271 should we mention C++03 as the de-facto standard for c++ code.

This comment has been minimized.

@scopatz

scopatz Jan 26, 2014

Member

Based on our discussion in #271 https://github.com/pyne/pyne/pull/271should we mention
C++03 as the de-facto standard for c++ code.

I am not sure we should really put this in the style guide because (1) I
can see this changing in the near future, and (2) I think we might make
frequent exceptions to this rule in a slow transition to C++11.

@scopatz

scopatz Jan 26, 2014

Member

Based on our discussion in #271 https://github.com/pyne/pyne/pull/271should we mention
C++03 as the de-facto standard for c++ code.

I am not sure we should really put this in the style guide because (1) I
can see this changing in the near future, and (2) I think we might make
frequent exceptions to this rule in a slow transition to C++11.

This comment has been minimized.

@crbates

crbates Jan 26, 2014

Contributor

Makes sense just thought I would mention it.

@crbates

crbates Jan 26, 2014

Contributor

Makes sense just thought I would mention it.

This comment has been minimized.

@scopatz

scopatz Jan 26, 2014

Member

Makes sense just thought I would mention it.

Uhmmmm thanks for doing so.

@scopatz

scopatz Jan 26, 2014

Member

Makes sense just thought I would mention it.

Uhmmmm thanks for doing so.

docs/devsguide/style_guide.rst
+**************
+Documentation
+**************
+In addition to folling the numpydoc convention, also include the function or method

This comment has been minimized.

@erelson

erelson Jan 26, 2014

Member

s/folling/following, and two lines below: s/sisnature/signature

@erelson

erelson Jan 26, 2014

Member

s/folling/following, and two lines below: s/sisnature/signature

@scopatz

This comment has been minimized.

Show comment
Hide comment
@scopatz

scopatz Jan 28, 2014

Member

Updates for variable name conventions based on debate with @elliottbiondo :)

Member

scopatz commented Jan 28, 2014

Updates for variable name conventions based on debate with @elliottbiondo :)

@scopatz

This comment has been minimized.

Show comment
Hide comment
@scopatz

scopatz Jan 28, 2014

Member

I wouldn't mind having more people look at this too. Though I think we are getting close to merging it in.

Member

scopatz commented Jan 28, 2014

I wouldn't mind having more people look at this too. Though I think we are getting close to merging it in.

+
+* ``xs`` stands for "cross section".
+* ``rx`` stands for "reaction".
+* ``ve`` stands for "volume element".

This comment has been minimized.

@elliottbiondo

elliottbiondo Jan 31, 2014

Member

So far in PyNE a "volume element" has meant a "mesh volume element". There are also geometry volume elements. I am not particularly concerned about this, but at a later time (depending on the future development of the dagmc module) we might want to switch to mve and gve.

@elliottbiondo

elliottbiondo Jan 31, 2014

Member

So far in PyNE a "volume element" has meant a "mesh volume element". There are also geometry volume elements. I am not particularly concerned about this, but at a later time (depending on the future development of the dagmc module) we might want to switch to mve and gve.

+* ``iso`` stands for an "isotope" in id form.
+* ``iso_name`` stands for an "isotope" in string form.
+* ``mat`` stands for "material".
+* ``m`` and ``mesh`` are used for Mesh instances.

This comment has been minimized.

@elliottbiondo

elliottbiondo Jan 31, 2014

Member

I really like the m idea. If a Mesh object is named "mesh" than accessing the iMesh instance is sort of confusing: mesh.mesh.

@elliottbiondo

elliottbiondo Jan 31, 2014

Member

I really like the m idea. If a Mesh object is named "mesh" than accessing the iMesh instance is sort of confusing: mesh.mesh.

+************
+* Code must have associated tests and adequate documentation.
+* The *only* exceptions to not having tests and documentation are when merging in and
+ slowly integrating legacy code or code not originally originally written for pyne.

This comment has been minimized.

This comment has been minimized.

@elliottbiondo

elliottbiondo Jan 31, 2014

Member

It looks like you use "pyne" throughout.

@elliottbiondo

elliottbiondo Jan 31, 2014

Member

It looks like you use "pyne" throughout.

@elliottbiondo

This comment has been minimized.

Show comment
Hide comment
@elliottbiondo

elliottbiondo Jan 31, 2014

Member

Though I am not yet knowledgeable enough to comment on the Cython portion, I think the rest of it is very good and ready to merge. I will do so if there are no objections.

Member

elliottbiondo commented Jan 31, 2014

Though I am not yet knowledgeable enough to comment on the Cython portion, I think the rest of it is very good and ready to merge. I will do so if there are no objections.

@gidden

This comment has been minimized.

Show comment
Hide comment
@gidden

gidden Jan 31, 2014

Contributor

I too think this is ready to pull

On Fri, Jan 31, 2014 at 12:55 PM, Elliott Biondo
notifications@github.comwrote:

Though I am not yet knowledgeable enough to comment on the Cython portion,
I think the rest of it is very good and ready to merge. I will do so if
there are no objections.

Reply to this email directly or view it on GitHubhttps://github.com/pyne/pyne/pull/257#issuecomment-33831047
.

Matthew Gidden
Ph.D. Candidate, Nuclear Engineering
The University of Wisconsin -- Madison
Ph. 225.892.3192

Contributor

gidden commented Jan 31, 2014

I too think this is ready to pull

On Fri, Jan 31, 2014 at 12:55 PM, Elliott Biondo
notifications@github.comwrote:

Though I am not yet knowledgeable enough to comment on the Cython portion,
I think the rest of it is very good and ready to merge. I will do so if
there are no objections.

Reply to this email directly or view it on GitHubhttps://github.com/pyne/pyne/pull/257#issuecomment-33831047
.

Matthew Gidden
Ph.D. Candidate, Nuclear Engineering
The University of Wisconsin -- Madison
Ph. 225.892.3192

elliottbiondo added a commit that referenced this pull request Feb 2, 2014

@elliottbiondo elliottbiondo merged commit aab6efe into pyne:staging Feb 2, 2014

@scopatz

This comment has been minimized.

Show comment
Hide comment
@scopatz

scopatz Feb 2, 2014

Member

Thanks all! I think we now have some cleanup work to do :)

Member

scopatz commented Feb 2, 2014

Thanks all! I think we now have some cleanup work to do :)

@scopatz scopatz deleted the scopatz:sg branch Feb 2, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment