Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

check type of attribute keys #1066

Merged
merged 10 commits into from
Sep 8, 2022
Merged

Conversation

malmans2
Copy link
Member

@malmans2 malmans2 commented Jul 4, 2022

Closes #1037
This PR adds a check that does not allow to use attribute keys other than strings.

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)

@codecov
Copy link

codecov bot commented Jul 4, 2022

Codecov Report

Merging #1066 (879eca2) into main (dcc6ded) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1066   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files          34       34           
  Lines       13846    13865   +19     
=======================================
+ Hits        13839    13858   +19     
  Misses          7        7           
Impacted Files Coverage Δ
zarr/attrs.py 100.00% <100.00%> (ø)
zarr/tests/test_attrs.py 100.00% <100.00%> (ø)

@malmans2
Copy link
Member Author

malmans2 commented Jul 4, 2022

I'm not sure whether we should add a note somewhere in the documentation/docstrings. Other than that, this PR is ready for review.

cc:
@joshmoore

@joshmoore
Copy link
Member

@malmans2 : I don't think I can update your branch with the latest origin/main. Could you do so to get all the tests passing?

@malmans2
Copy link
Member Author

@malmans2 : I don't think I can update your branch with the latest origin/main. Could you do so to get all the tests passing?

Done! Let me know if we should start a deprecation cycle.

@joshmoore
Copy link
Member

Happy if someone who wants to be adventurous speaks up, but I'd err on the side of the deprecation cycle.

@joshmoore
Copy link
Member

Sorry for the delay, @malmans2, and thanks for the deprecation cycle. If we're temporarily accepting the stringification, would you be able to workaround your TypeError by explicitly calling str()?

zarr/attrs.py Show resolved Hide resolved
@joshmoore
Copy link
Member

Thanks, @malmans2! Rolling into 2.13

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.

Error when using attribute keys with mixed types
2 participants