I think the easiest way forward is to do a follow-up PR, no need to revert.

Reverting would be helpful if we wanted to undo these changes - in this case we have some v marginal additional changes.

My (limited) understanding of how github handles reverting is that you'll still need to make a new pull request to apply your edits either way. There is no more going to back to amend after PR is merges. The only difference is what the state of "master" looks like when you make your new pull request (whether it includes your partial changes or not).

So I don't think it particularly matters if you revert first (as long as the forward fix happens relatively soon), but if you'd prefer to do it that way that's totally fine -- let me know and I will do it.

Happy to make these changes, but what's the protocol here? Seems like reverting the merge and letting me amend is cleaner than me creating a new PR.

Post merge reviews are totally fine. It is not a big deal to revert a merge if needed.

@shoyer thanks for the follow-up comments. I probably should have spotted those myself. But given there'll be points I miss, lmk if I should be waiting longer / raising specific points before hitting the big green button, or these post-merge reviews are OK. I can ensure they're complete before a release.

Great @gwgundersen !

@max-sixty, done.

Hello @gwgundersen! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2019-08-18 07:25:58 UTC

@gwgundersen could we add a whatsnew too?

Sorry for the back-and-forth. CI tests and checks are all passing, and this is ready for another review.

I see the docs timing out after 60 min. I also see that message, but is that causing the failure?

I restarted the docs build to see whether that might help

It looks like you've added a warming for this case. So you'll need to specify :okwarning: .

Now the docs don't build. When I try to build locally, I get a vague error:

bash File "/Users/gwg/miniconda3/envs/xarray-docs/lib/python3.7/site-packages/IPython/sphinxext/ipython_directive.py", line 583, in process_input raise RuntimeError('Non Expected warning in `{}` line {}'.format(filename, lineno)) RuntimeError: Non Expected warning in `/Users/gwg/projects/xarray/doc/indexing.rst` line 241

Do you see anything wrong with this?

``rest The :py:meth:~xarray.Dataset.drop` method returns a new object with the listed index labels along a dimension dropped:

.. ipython:: python

ds.drop(['IN', 'IL'], dim='space')

<-- line 241 drop is both a Dataset and DataArray method.

Use :py:meth:~xarray.Dataset.drop_dims to drop a full dimension from a Dataset. Any variables with these dimensions are also dropped: ```

Thanks @gwgundersen - good idea re the cherry pick for more discrete changes.

Still getting a test failure though, check it out and lmk if you need any help

Sorry about that. I'm not sure what happened. I've reverted branch and cherry picked my changes. The issue with drop being a duplicated name still stands.

Codecov Report

Merging #3128 into master will increase coverage by 0.31%. The diff coverage is 92.85%.

```diff @@ Coverage Diff @@

master #3128 +/-

========================================== + Coverage 95.66% 95.98% +0.31%
========================================== Files 64 63 -1
Lines 13117 12788 -329
========================================== - Hits 12548 12274 -274
+ Misses 569 514 -55 ```

@gwgundersen Could you force-push your latest code?

If your current latest code is pushed, you may have to git reset --hard and start again - unfortunately it looks like your PR is proposing +24,439 −17,647 changes

@max-sixty, thanks! It's useful to be able to do in advance.

After merging from master, I get this error:

flake8 ./xarray/core/dataset.py:3466:5: F811 redefinition of unused 'drop' from line 3460

I assume this is due to https://github.com/pydata/xarray/pull/3177 cc @crusaderky. Is there a way to avoid this?

@gwgundersen I see you trying to merge re the recent black changes. Here are the instructions for making this (relatively) seamless: https://github.com/pydata/xarray/blob/d3f1ced1aa84cb0a0f3e47e7ee67492e203fcce5/doc/contributing.rst#code-formatting

Awesome, thanks for working on this @gwgundersen !

In the long term, we might want to deprecate the current function signature, but I'm happy to leave that for later.