Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Make
create_array
signatures consistent #2819New 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
base: main
Are you sure you want to change the base?
Make
create_array
signatures consistent #2819Changes from all commits
210f4e9
4c966c0
665eebb
918f86a
280cb6b
642272d
3fa6526
0f9e69e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So to clarify, this PR is a breaking one for writing v2 data, which changes what is written as the fill value if the user doesn't specify it? And this changed test is the manifestation of that breaking change? Is this something we want to avoid breaking?
(sorry for all the questions, just trying to understand 😄 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a change to how v2 metadata is written. the actual array data will be written as before. Because the signatures of the affected functions are inconsistent, there is no way to achieve the goal of this PR without changing the default behavior of some of those functions, which will result in different metadata being written to disk as an effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so to your questions: it's breaking if someone relied on the default metadata document produced by some, but not all, of our array creation routines. And no, we do not want to avoid breaking, because the cost of a confusing, inconsistent API is worth paying in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to all that. In which case this is a backwards incompatible API change, and we promise to increment the major version number for backwards incompatible API changes. So, what do we want to do - bump the version number v4, or re-write the versioning policy?
I'm guessing it's the latter... which is a pain, but I think we should be honest and transparent to users if we're going to put backwards incompatible API changes in minor releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should adjust the versioning policy, and also release the changes in this PR in a minor (non-patch) release. We were already planning on making breaking changes without incrementing the major version number -- deprecated functions like
create
were slated to be removed long before a 4.x release. So I think the versioning policy needs updating.