Skip to content

Conversation

DimitriPapadopoulos
Copy link
Contributor

Detected by pyugrade.

TODO:

  • Unit tests and/or doctests in docstrings
  • Tests pass locally
  • Docstrings and API docs for any new/modified user-facing classes and functions
  • Changes documented in docs/release.rst
  • Docs build locally
  • GitHub Actions CI passes
  • Test coverage to 100% (Coveralls passes)

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Dimitri! 🙏

Had one comment below

_json.dump(codec.get_config(), cf, sort_keys=True, indent=4)
# load config and compare with expectation
with open(codec_fn, mode='r') as cf:
with open(codec_fn) as cf:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we keep the mode here? Explicit is better than implicit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand, but pyupgrade or refurb disagree and believe implicit is better than explicit 😄

I believe the rationale is that open() has many default arguments, and all of them are implicit (buffering, encoding, errors, newline, closefd, opener). That's why Python has default arguments. Why make an exception for mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then I don't have strong opinions on the subject. I was myself rather reluctant initially:
codespell-project/codespell#2364 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah mode seems special since it affects whether the file may be modified (or even truncated) when opening

That said, not too worried about it given the placement (in the tests)

Have occasionally run into unexpected default behavior around mode in some libraries. Hence the initial pause

Detected by pyugrade.
@codecov
Copy link

codecov bot commented Nov 6, 2022

Codecov Report

Merging #387 (46ce739) into main (91d05e4) will increase coverage by 0.38%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##             main      #387      +/-   ##
===========================================
+ Coverage   99.61%   100.00%   +0.38%     
===========================================
  Files          54        53       -1     
  Lines        2101      2024      -77     
===========================================
- Hits         2093      2024      -69     
+ Misses          8         0       -8     
Impacted Files Coverage Δ
numcodecs/ndarray_like.py 100.00% <ø> (+12.50%) ⬆️
numcodecs/tests/test_registry.py 100.00% <ø> (+4.34%) ⬆️
numcodecs/zfpy.py 100.00% <ø> (+2.43%) ⬆️
numcodecs/tests/common.py 100.00% <100.00%> (ø)
numcodecs/__init__.py

@jakirkham jakirkham enabled auto-merge (squash) November 6, 2022 02:23
@jakirkham jakirkham disabled auto-merge November 7, 2022 20:32
@jakirkham jakirkham enabled auto-merge (squash) November 7, 2022 20:54
@jakirkham jakirkham merged commit 3b9129f into zarr-developers:main Nov 7, 2022
@DimitriPapadopoulos DimitriPapadopoulos deleted the pyugrade branch November 7, 2022 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants