-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Minor: improve the Deprecation / API health guidelines #13701
Changes from all commits
b109ba3
c9ba507
38aa52c
9d63f3a
bb12c94
5e221a5
83a9e58
1ba657e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -19,13 +19,51 @@ | |||||
|
||||||
# API health policy | ||||||
|
||||||
To maintain API health, developers must track and properly deprecate outdated methods. | ||||||
DataFusion is used extensively as a library and has a large public API, thus it | ||||||
is important that the API is well maintained. In general, we try to minimize | ||||||
breaking API changes, but they are sometimes necessary. | ||||||
|
||||||
When possible, rather than making breaking API changes, we prefer to deprecate | ||||||
APIs to give users time to adjust to the changes. | ||||||
|
||||||
## Breaking Changes | ||||||
|
||||||
In general, a function is part of the public API if it appears on the [docs.rs page] | ||||||
|
||||||
Breaking public API changes are those that _require_ users to change their code | ||||||
for it to compile, and are listed as "Major Changes" in the [SemVer | ||||||
Compatibility Section of the cargo book]. Common examples of breaking changes: | ||||||
|
||||||
- Adding new required parameters to a function (`foo(a: i32, b: i32)` -> `foo(a: i32, b: i32, c: i32)`) | ||||||
- Removing a `pub` function | ||||||
- Changing the return type of a function | ||||||
|
||||||
When making breaking public API changes, please add the `api-change` label to | ||||||
the PR so we can highlight the changes in the release notes. | ||||||
|
||||||
[docs.rs page]: https://docs.rs/datafusion/latest/datafusion/index.html | ||||||
[semver compatibility section of the cargo book]: https://doc.rust-lang.org/cargo/reference/semver.html#change-categories | ||||||
|
||||||
## Deprecation Guidelines | ||||||
|
||||||
When deprecating a method: | ||||||
|
||||||
- clearly mark the API as deprecated and specify the exact DataFusion version in which it was deprecated. | ||||||
- concisely describe the preferred API, if relevant | ||||||
- Mark the API as deprecated using `#[deprecated]` and specify the exact DataFusion version in which it was deprecated | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make it more copy-paste friendly
this may be important given that there is no static check which would flag There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (opt)
Suggested change
(then you don't need the code block below) |
||||||
- Concisely describe the preferred API to help the user transition | ||||||
|
||||||
The deprecated version is the next version which contains the deprecation. For | ||||||
example, if the current version listed in [`Cargo.toml`] is `43.0.0` then the next | ||||||
version will be `44.0.0`. | ||||||
|
||||||
[`cargo.toml`]: https://github.com/apache/datafusion/blob/main/Cargo.toml | ||||||
|
||||||
To mark the API as deprecated, use the `#[deprecated]` attribute like this: | ||||||
|
||||||
```rust | ||||||
#[deprecated(since = "...", note = "...")] | ||||||
``` | ||||||
|
||||||
API deprecation example: | ||||||
For example: | ||||||
|
||||||
```rust | ||||||
#[deprecated(since = "41.0.0", note = "Use SessionStateBuilder")] | ||||||
|
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.
Not "to compile" but rather "to compile and execute"
for example replacing the function body with a
todo!()
is also a breaking change, even though it does not affect the compile time.See #13525 (comment) -- we should not motivate people to avoid compile-time breakages as this may come at the cost of harder to find runtime-breakages (as witnessed in #13708 (comment))
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.
bump