io.open → open

[DimitriPapadopoulos]

In Python 3, io.open() is an alias for the builtin open() function:
https://docs.python.org/3/library/io.html#io.open

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/tutorial.rst
• Changes documented in docs/release.rst
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[DimitriPapadopoulos]

That's disturbing. Despite io.open() being (supposedly) an alias for open(), changing to open() fails. The error message is unclear, that's perhaps a real issue.

[DimitriPapadopoulos]

Got it. File convenience.py defines a function open that shadows the builtin open 🤯

zarr-python/zarr/convenience.py

Line 34 in 4132f36

def open(store: StoreLike = None, mode: str = "a", *, zarr_version=None, path=None, **kwargs):

Not really convenient if you ask me.

[DimitriPapadopoulos]

This should probably be fixed by changing the name of the local function open, perhaps to zarr_open.

Then change:

zarr-python/zarr/__init__.py

Lines 3 to 5 in 4132f36

	from zarr.convenience import (consolidate_metadata, copy, copy_all, copy_store,
	load, open, open_consolidated, save, save_array,
	save_group, tree)

to:

from zarr.convenience import (consolidate_metadata, copy, copy_all, copy_store,
                              load, open_consolidated, save, save_array,
                              save_group, tree)
from zarr.convenience import zarr_open as open

Thoughts?

[codecov]

Codecov Report

Merging #1421 (9017ac2) into main (1ed37f5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 9017ac2 differs from pull request most recent head 2478f46. Consider uploading reports for the commit 2478f46 to get more accurate results

Additional details and impacted files

@@           Coverage Diff            @@
##             main     #1421   +/-   ##
========================================
  Coverage   99.99%   100.00%           
========================================
  Files          37        37           
  Lines       14740     14735    -5     
========================================
- Hits        14739     14735    -4     
+ Misses          1         0    -1     

Files	Coverage Δ
zarr/convenience.py	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

[d-v-b]

@DimitriPapadopoulos

This should probably be fixed by changing the name of the local function open, perhaps to zarr_open.

I have no objection to this strategy. Do you want to pursue that in this PR, or elsewhere?

[DimitriPapadopoulos]

Probably another PR.

After thinking more about it, nothing's wrong with an open function, it should just be used as
zarr.convenience.open or from zarr.convenience import open as zarr_open.

[rabernat]

Are we certain this is the only use of io.open?

[DimitriPapadopoulos]

I think so. A grep of io.open yields:

$ fgrep -R io.open --exclude-dir=.git
zarr/convenience.py:            self.log_file = io.open(log, mode="w")
build/lib/zarr/convenience.py:            self.log_file = io.open(log, mode='w')
$ 

And a grep of open( in zarr/convenience.py yields:

$ grep -R 'open(' zarr/convenience.py 
def open(store: StoreLike = None, mode: str = "a", *, zarr_version=None, path=None, **kwargs):
        >>> zw = zarr.open(store, mode='w', shape=100, dtype='i4')  # open new array
        >>> za = zarr.open(store, mode='a')  # open existing array for reading and writing
        >>> zr = zarr.open(store, mode='r')  # open existing array read-only
        >>> gw = zarr.open(store, mode='w')  # open new group, overwriting previous data
        >>> ga = zarr.open(store, mode='a')  # open existing group for reading and writing
        >>> gr = zarr.open(store, mode='r')  # open existing group read-only
            self.log_file = io.open(log, mode="w")
    return open(store=meta_store, chunk_store=chunk_store, mode=mode, path=path, **kwargs)
$ 

Do you have a specific concern, anything I may have overlooked?

[rabernat]

No not really. Just wanted to make sure we only have to do this once.

[rabernat]

Thanks a lot for this PR!