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

Minor: improve the Deprecation / API health guidelines #13701

Merged
merged 8 commits into from
Dec 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -140,5 +140,8 @@ DataFusion enforces MSRV policy using a [MSRV CI Check](https://github.com/searc

## DataFusion API evolution policy

Public methods in Apache DataFusion are subject to evolve as part of the API lifecycle.
Deprecated methods will be phased out in accordance with the [policy](https://datafusion.apache.org/library-user-guide/api-health.html), ensuring the API is stable and healthy.
Public methods in Apache DataFusion evolve over time: while we try to maintain a
stable API, we also improve the API over time. As a result, we typically
deprecate methods before removing them, according to the [api health policy].

[api health policy]: https://datafusion.apache.org/library-user-guide/api-health.html
46 changes: 42 additions & 4 deletions docs/source/library-user-guide/api-health.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines +33 to +34
Copy link
Member

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))

Copy link
Member

Choose a reason for hiding this comment

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

bump

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
Copy link
Member

Choose a reason for hiding this comment

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

Make it more copy-paste friendly

#[deprecated(since = "...",  note = "...")]

this may be important given that there is no static check which would flag #[deprecated] without since attribute and those sometimes make to the main branch (apache/arrow-rs#6782)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Member

Choose a reason for hiding this comment

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

(opt)

Suggested change
- Mark the API as deprecated using `#[deprecated]` and specify the exact DataFusion version in which it was deprecated
- Mark the API as deprecated using `#[deprecated(since="...", note="..."]` and specify the exact DataFusion version in which it was deprecated

(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")]
Expand Down
Loading