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

Use ChunkKeyEncodingLike type alias #2763

Merged
merged 6 commits into from
Jan 29, 2025

Conversation

d-v-b
Copy link
Contributor

@d-v-b d-v-b commented Jan 24, 2025

previously, ChunkKeyEncodingLike was the name of a TypedDict instance that models the JSON form of the chunk_key_encoding attribute in zarr v3 array metadata. But ChunkKeyEncodingLike should be the name of a type alias that denotes the union of types that can be converted to an instance of ChunkKeyEncoding. So I renamed the typeddict to ChunkKeyEncodingParams and added a type alias (it is ChunkKeyEncoding | ChunkKeyEncodingParams)

@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Jan 24, 2025
@d-v-b d-v-b marked this pull request as draft January 24, 2025 18:00
@d-v-b d-v-b marked this pull request as ready for review January 24, 2025 18:46
@d-v-b
Copy link
Contributor Author

d-v-b commented Jan 25, 2025

@dstansby can you check that my release notes are correct? and is it possible for the gha that adds the needs release notes label also remove that label when release notes are added?

@dstansby
Copy link
Contributor

Looks good to me! Perhaps the note could say how the change impacts users? I'm guessing here the typing is just being made more permissive?

and is it possible for the gha that adds the needs release notes label also remove that label when release notes are added?

Yes, it looks like there's a sync-labels option for the labeler action: https://github.com/actions/labeler/tree/main/?tab=readme-ov-file#inputs

@d-v-b
Copy link
Contributor Author

d-v-b commented Jan 26, 2025

It should have no effect on users.

@dstansby
Copy link
Contributor

In that case I'd probably skip adding a release note? At least in my mind, release notes are targeted other folks using the package, not internally for developers.

@d-v-b
Copy link
Contributor Author

d-v-b commented Jan 26, 2025

developers are also users :)

@d-v-b
Copy link
Contributor Author

d-v-b commented Jan 26, 2025

in fact, I think developers will be more interested in reading the release notes than ordinary users. Thus I don't see a particular reason to omit infrastructure changes from the release notes.

@dstansby
Copy link
Contributor

I guess so - in that case LGTM, and perhaps the 'chore' release notes category is a good place to put developer facing notes 👍

@d-v-b d-v-b enabled auto-merge (squash) January 26, 2025 19:49
@d-v-b d-v-b requested a review from dstansby January 29, 2025 17:10
@d-v-b d-v-b removed the needs release notes Automatically applied to PRs which haven't added release notes label Jan 29, 2025
@d-v-b d-v-b merged commit e602aa1 into zarr-developers:main Jan 29, 2025
30 checks passed
@d-v-b d-v-b deleted the fix/chunkkeyencodinglike branch January 29, 2025 22:44
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.

3 participants