Code cleanup #5234

andersy005 · 2021-04-29T05:16:02Z

This PR introduces small changes to enhance code maintainability...

Passes pre-commit run --all-files

max-sixty

mathause

Thanks @andersy005 - I am not so sure about the inline dicts: once they begin to span more than 2 lines I find them more difficult than helpful.

xarray/backends/api.py

xarray/backends/netCDF4_.py

xarray/coding/strings.py

xarray/backends/scipy_.py

xarray/coding/strings.py

xarray/core/coordinates.py

xarray/core/utils.py

mathause · 2021-04-29T08:19:44Z

xarray/plot/utils.py

    else:
-        extend = "neither"
-    return extend
+        return "neither"


(I don't like the pattern so much but you could remove the else).

xarray/backends/api.py

keewis

thanks, @andersy005.

The multiline dict comprehensions are a matter of taste (I do prefer them over item assignment in a loop where possible), but as far as I can tell the only difference is the speed of the loop evaluation in CPython (dict comprehensions iterate in C)

xarray/backends/api.py

xarray/backends/h5netcdf_.py

xarray/backends/netCDF4_.py

xarray/backends/plugins.py

xarray/core/accessor_dt.py

xarray/core/accessor_str.py

xarray/core/alignment.py

keewis · 2021-04-29T14:19:01Z

xarray/core/coordinates.py

+        level_list = []
+        names = []
+
+        for i, index in enumerate(indexes):


since i is only used once, you could also use zip(indexes, repeat_counts) instead of enumerate(...)

xarray/core/utils.py

xarray/core/variable.py

Co-authored-by: keewis <keewis@users.noreply.github.com> Co-authored-by: Mathias Hauser <mathause@users.noreply.github.com>

xarray/backends/h5netcdf_.py

xarray/backends/netCDF4_.py

xarray/coding/strings.py

xarray/core/accessor_str.py

xarray/backends/api.py

Co-authored-by: Stephan Hoyer <shoyer@google.com>

xarray/backends/netCDF4_.py

alexamici · 2021-05-04T07:32:32Z

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.

xarray/backends/api.py

max-sixty · 2021-05-04T08:37:16Z

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?

alexamici · 2021-05-04T11:02:15Z

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

andersy005 · 2021-05-06T13:24:18Z

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 :)

xarray/backends/netCDF4_.py

xarray/backends/h5netcdf_.py

xarray/backends/netCDF4_.py

xarray/backends/plugins.py

xarray/backends/zarr.py

xarray/coding/variables.py

xarray/core/duck_array_ops.py

xarray/core/groupby.py

xarray/core/merge.py

xarray/core/rolling.py

shoyer · 2021-05-07T05:53:19Z

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

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

andersy005 · 2021-05-07T22:20:00Z

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

max-sixty · 2021-05-12T15:39:56Z

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

andersy005 · 2021-05-12T15:44:51Z

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

dcherian · 2021-05-12T15:52:23Z

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

andersy005 · 2021-05-12T16:07:29Z

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

👍🏽 for addressing this in a separate PR

andersy005 · 2021-05-12T16:15:13Z

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

xarray/coding/variables.py

dcherian · 2021-05-13T16:37:04Z

Looks like this is done. Thanks @andersy005

* upstream/master: combine keep_attrs and combine_attrs in apply_ufunc (pydata#5041) Explained what a deprecation cycle is (pydata#5289) Code cleanup (pydata#5234) FacetGrid docstrings (pydata#5293) Add whats new for dataset interpolation with non-numerics (pydata#5297) Allow dataset interpolation with different datatypes (pydata#5008) Flexible indexes: add Index base class and xindexes properties (pydata#5102) pre-commit: autoupdate hook versions (pydata#5280) convert the examples for apply_ufunc to doctest (pydata#5279) fix the new whatsnew section Ensure `HighLevelGraph` layers are `Layer` instances (pydata#5271)

* upstream/master: (23 commits) combine keep_attrs and combine_attrs in apply_ufunc (pydata#5041) Explained what a deprecation cycle is (pydata#5289) Code cleanup (pydata#5234) FacetGrid docstrings (pydata#5293) Add whats new for dataset interpolation with non-numerics (pydata#5297) Allow dataset interpolation with different datatypes (pydata#5008) Flexible indexes: add Index base class and xindexes properties (pydata#5102) pre-commit: autoupdate hook versions (pydata#5280) convert the examples for apply_ufunc to doctest (pydata#5279) fix the new whatsnew section Ensure `HighLevelGraph` layers are `Layer` instances (pydata#5271) New whatsnew section Release-workflow: Bug fix (pydata#5273) more maintenance on whats-new.rst (pydata#5272) v0.18.0 release highlights (pydata#5266) Fix exception when display_expand_data=False for file-backed array. (pydata#5235) Warn ignored keep attrs (pydata#5265) Disable workflows on forks (pydata#5267) fix the built wheel test (pydata#5270) pypi upload workflow maintenance (pydata#5269) ...

…e_units * upstream/master: combine keep_attrs and combine_attrs in apply_ufunc (pydata#5041) Explained what a deprecation cycle is (pydata#5289) Code cleanup (pydata#5234) FacetGrid docstrings (pydata#5293) Add whats new for dataset interpolation with non-numerics (pydata#5297) Allow dataset interpolation with different datatypes (pydata#5008) Flexible indexes: add Index base class and xindexes properties (pydata#5102)

* upstream/master: Update release guide (pydata#5274) combine keep_attrs and combine_attrs in apply_ufunc (pydata#5041) Explained what a deprecation cycle is (pydata#5289) Code cleanup (pydata#5234) FacetGrid docstrings (pydata#5293) Add whats new for dataset interpolation with non-numerics (pydata#5297) Allow dataset interpolation with different datatypes (pydata#5008)

max-sixty · 2021-05-13T18:05:12Z

Thanks @andersy005 !

andersy005 added 5 commits April 28, 2021 22:30

backends code cleanup

630ac54

Coding module cleanup

d68ede4

Core module cleanup

a4f0346

Misc code cleanup

46c24ee

Rename extracted method

5d5f7a4

max-sixty approved these changes Apr 29, 2021

View reviewed changes

mathause approved these changes Apr 29, 2021

View reviewed changes

keewis reviewed Apr 29, 2021

View reviewed changes

Apply suggestions from code review

8c818df

Co-authored-by: keewis <keewis@users.noreply.github.com> Co-authored-by: Mathias Hauser <mathause@users.noreply.github.com>

This comment has been minimized.

Sign in to view

formatting only

12141d1

shoyer reviewed Apr 29, 2021

View reviewed changes

andersy005 and others added 4 commits April 29, 2021 13:49

Use f-Strings

99a54de

Apply suggestions from code review

ee34741

Co-authored-by: Stephan Hoyer <shoyer@google.com>

Revert back to original versions

4d6e78e

Revert back to original version

1cc604f

andersy005 commented Apr 30, 2021

View reviewed changes

xarray/backends/netCDF4_.py Show resolved Hide resolved

alexamici reviewed May 4, 2021

View reviewed changes

xarray/backends/api.py Outdated Show resolved Hide resolved

andersy005 added 2 commits May 6, 2021 06:59

Merge branch 'master' of github.com:pydata/xarray into backends-cleanup

58e1b2b

Address @alexamici 's comment

e7b5e84

shoyer requested changes May 7, 2021

View reviewed changes

andersy005 added 3 commits May 7, 2021 15:17

Merge branch 'master' of github.com:pydata/xarray into backends-cleanup

9514b1a

Revert back to original versions

879e2f5

Revert back to original versions

e853ee8

andersy005 added 2 commits May 12, 2021 09:54

Merge branch 'master' of github.com:pydata/xarray into backends-cleanup

f067422

Revert back to original version

ca769d4

shoyer approved these changes May 12, 2021

View reviewed changes

xarray/coding/variables.py Outdated Show resolved Hide resolved

Fix missing empty line

9024b30

dcherian merged commit 1a7b285 into pydata:master May 13, 2021

andersy005 deleted the backends-cleanup branch June 16, 2023 22:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code cleanup #5234

Code cleanup #5234

andersy005 commented Apr 29, 2021 •

edited

Loading

max-sixty left a comment

mathause left a comment

mathause Apr 29, 2021

keewis left a comment

keewis Apr 29, 2021

This comment has been minimized.

alexamici commented May 4, 2021

max-sixty commented May 4, 2021

alexamici commented May 4, 2021

andersy005 commented May 6, 2021

shoyer commented May 7, 2021

andersy005 commented May 7, 2021

max-sixty commented May 12, 2021

andersy005 commented May 12, 2021

dcherian commented May 12, 2021

andersy005 commented May 12, 2021

andersy005 commented May 12, 2021

dcherian commented May 13, 2021

max-sixty commented May 13, 2021

Code cleanup #5234

Code cleanup #5234

Conversation

andersy005 commented Apr 29, 2021 • edited Loading

max-sixty left a comment

Choose a reason for hiding this comment

mathause left a comment

Choose a reason for hiding this comment

mathause Apr 29, 2021

Choose a reason for hiding this comment

keewis left a comment

Choose a reason for hiding this comment

keewis Apr 29, 2021

Choose a reason for hiding this comment

This comment has been minimized.

alexamici commented May 4, 2021

max-sixty commented May 4, 2021

alexamici commented May 4, 2021

andersy005 commented May 6, 2021

shoyer commented May 7, 2021

andersy005 commented May 7, 2021

max-sixty commented May 12, 2021

andersy005 commented May 12, 2021

dcherian commented May 12, 2021

andersy005 commented May 12, 2021

andersy005 commented May 12, 2021

dcherian commented May 13, 2021

max-sixty commented May 13, 2021

andersy005 commented Apr 29, 2021 •

edited

Loading