issue_comments

BTW, I think I messed up my co-worker's name in whats-new.rst. The spelling is right (Kyle Heuton), but the extra indentation shouldn't be there. As a result his name appears a bit out of place. Would be very much obliged if you could fix that in your next push to master. Gracias.

No worries, whats-new.rst usually requires a fix-up before each release

api.rst should've been a checklist item :)

Indeed, I will add it right now!

Thanks for your contribution!!

One last thing -- this needs an entry in docs/api.rst in order for the doc page to get built.

It's very strange that fake8 complains about non-Python files. I think you can safely ignore those. On Wed, May 10, 2017 at 8:55 PM chunweiyuan notifications@github.com wrote:

git diff upstream/master | flake8 --diff only complains about a bunch of lines in whats-new.rst and reshaping.rst. But these complaints don't make any sense to me. Some examples:

doc/reshaping.rst:195:1: E112 expected an indented block doc/reshaping.rst:203:1: E101 indentation contains mixed spaces and tabs doc/reshaping.rst:203:1: W191 indentation contains tabs doc/reshaping.rst:203:2: E113 unexpected indentation

I've played around with it a bit but not seen any changes to the complaints. What is going on? Should I even worry about the .rst files?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/pull/1389#issuecomment-300675689, or mute the thread https://github.com/notifications/unsubscribe-auth/ABKS1qjB4dK0tHLBMrX6sURnlt_T5QX0ks5r4obPgaJpZM4NMGMU .

Scroll to the top of this PR and look at your first post :)

On Wed, May 10, 2017 at 11:08 AM, chunweiyuan notifications@github.com wrote:

I just made some futile attempts to find that checklist, to no avail. Mind helping me with a link? :)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/pull/1389#issuecomment-300566860, or mute the thread https://github.com/notifications/unsubscribe-auth/ABKS1mF7Jjb2NmZ_5LZpRvUP_nwG-khgks5r4f0hgaJpZM4NMGMU .

Today is the first time I've heard of flake8.

It's in the checklist for every new PR :).

Is this how you guys standardize quality checks?

Yes. It would be nice to run this as a continuous integration test but we haven't set that up that.

Running it once with either Python 2 or 3 is fine -- it should give the same output either way.

@chunweiyuan have you run flake8 on this?

Ah, good to know.

In that case, let's add a check for dtype == object based on numpy.__version__ (search for LooseVersion for examples in the xarray codebase) and raise an informative NotImplementedError when necessary.

If I downgrade to numpy 1.10, I get that error. Once upgraded to 1.12, it goes away......

Does it work on numpy 1.11?

We could potentially (partially) drop compatibility for older numpy releases. Or if we can identify the issue, we can raise an informative error or use a work-around.

Looks like one the Travis-CI tests failing on the sortby tests: https://travis-ci.org/pydata/xarray/jobs/229457729#L342

It's not immediately clear to me what's going on there, but maybe it's an issue with the old version of NumPy? The error is TypeError: merge sort not available for item 0.

Indeed, please ignore the first CI build on Travis. For some inexplicable reason it is installing the pandas 0.20rc, which is failing due to https://github.com/pydata/xarray/issues/1386.

I am slightly in favor of sortby but also could go either way. The PEP8 guidance on function/method naming is "lowercase with words separated by underscores as necessary to improve readability." I don't think an underscore is necessary for readability here.