Minor: improve the Deprecation / API health guidelines #13701
Conversation
alamb
changed the title
github-actions
bot
added
the
documentation
Co-authored-by: Jonah Gao <[email protected]>
| 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 |
There was a problem hiding this comment.
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))
| 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.
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)
There was a problem hiding this comment.
(opt)
| - 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)
alamb
changed the title
|
@alamb thanks for improving this! |
Sorry about missing that @findepi. I made another PR to address it |
Which issue does this PR close?
Rationale for this change
While discussing #13037 (comment) with @buraksenn I felt the API deprecation policy could be improved to provide more background and rationale
What changes are included in this PR?
Are these changes tested?
CI
Are there any user-facing changes?
I my opinion, this PR does not change the policy, it simply makes the current policy clearer