issue_comments

Thanks @andersy005 !

Looks like this is done. Thanks @andersy005

I believe I have addressed most, if not all changes requested during the review. If there are still disputable changes that need addressing, please let me know

Shall we also add parts of Stephan's comment to the "Contributing Guide" in the documentation?

👍🏽 for addressing this in a separate PR

Shall we also add parts of Stephan's comment to the "Contributing Guide" in the documentation?

@max-sixty, I'm going to address a few issues pointed out during the review, then we can merge afterwards.

This is going to collect merge conflicts — shall we merge?

@shoyer, Thank you for the thorough feedback.

Even code that has been carefully checked can introduce bugs. Style clean-up PRs thus still need to be reviewed carefully.

Fewer lines or tokens of code is not always better. Good examples from this PR include reduced nesting with early return statements, comprehensions and conditional expressions. All of these would be unobjectionable to use in new code (and are often a good idea), but code that doesn't use them even when it could is fine, too.

👍🏽. I reverted back to the original style in most places. There're a few comments that I haven't addressed yet. I will look into those sometime this weekend.

Code-base wide clean-up for particular issues may be warranted occasionally, but I would suggest always discussing it with other core developers first.

I agree. I didn't expect the PR to get this large. My original intent when I created the PR was to clean the backends code, and I ended up invading other areas :(. In the future, I'll definitely make sure to start a discussion before working on changes that may end up affecting a large chunk of the code base.

@andersy005 thanks for your hard work here. On the whole, I think this will be a net benefit to the codebase, but in my review I pointed out a number of locations where I did not find the style to be improved (in my opinion). There is also one place where I think your logic may have an error.

It would be nice to come up with a policy around "style clean-up" PRs for the future. In general, I think these are not a good idea, for several reasons:

1. Even code that has been carefully checked can introduce bugs. Style clean-up PRs thus still need to be reviewed carefully.
2. Fewer lines or tokens of code is not always better. Good examples from this PR include reduced nesting with early return statements, comprehensions and conditional expressions. All of these would be unobjectionable to use in new code (and are often a good idea), but code that doesn't use them even when it could is fine, too.
3. Some ugly code works fine, and would not benefit from clean-up. For example, xarray.util.print_versions (which was forked from pandas) is quite ugly code. But it's peripheral to the project, and has barely been touched aside from automated reformatting.

Instead, I would suggest a policy of a selective cleanup only. Feel free to clean-up style when it arises in the process of normal coding, e.g., when you are modifying nearly or related code for another reason. This reduces the risk of introducing inadvertent errors, and the fact that you were working on nearby code means that this part of the codebase is worthy of improvement.

Code-base wide clean-up for particular issues may be warranted occasionally, but I would suggest always discussing it with other core developers first.

I feel this PR is too big and diverse to accept it as a whole.

For example I like most changes from older style formatting to f-strings, but I disagree strongly the moving the return statements into functions improves readability.

If folks are in favor of the original style (inline variables that are immediately returned) throughout the codebase, I'm happy to revert back to the original style :)

@max-sixty & @andersy005 mine was more of a general comment. It is OK to merge it for me, but please after the release!

I feel this PR is too big and diverse to accept it as a whole.

For example I like most changes from older style formatting to f-strings, but I disagree strongly the moving the return statements into functions improves readability.

While I don't have the same sense on the specifics, if people feel strongly then it may be reasonable to revert or pause on some of these.

What do you suggest re the broader PR though? While focused PRs are easier to review and reach agreement — now this is here, it does seem on net beneficial, and we should take advantage of the benefit.

Are there a few items you feel strongly about that we could revert and then merge the rest?

I feel this PR is too big and diverse to accept it as a whole.

For example I like most changes from older style formatting to f-strings, but I disagree strongly the moving the return statements into functions improves readability.