issue_comments: 834087321

This data as json

html_url issue_url id node_id user created_at updated_at author_association body reactions performed_via_github_app issue
https://github.com/pydata/xarray/pull/5234#issuecomment-834087321 https://api.github.com/repos/pydata/xarray/issues/5234 834087321 MDEyOklzc3VlQ29tbWVudDgzNDA4NzMyMQ== 1217238 2021-05-07T05:53:19Z 2021-05-07T05:53:19Z MEMBER

@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.

 
{
    "total_count": 2,
    "+1": 2,
    "-1": 0,
    "laugh": 0,
    "hooray": 0,
    "confused": 0,
    "heart": 0,
    "rocket": 0,
    "eyes": 0
}
   870619014