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

Add deprecation / API removal policy #6852

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Dec 6, 2024

Which issue does this PR close?

Closes #6851

Rationale for this change

As this crate matures, more predictability is good

What changes are included in this PR?

Add a proposed deprecation policy to the README

Options considered:

  • at least one major release ==> 3-6 months
  • 2 major releases ==> 6 - 9 months.

I am currently proposing 2 major releases, but would love to hear more input

Are there any user-facing changes?

Just docs, but new policy

@alamb alamb assigned alamb and unassigned alamb Dec 6, 2024
@alamb alamb added parquet Changes to the parquet crate arrow Changes to the arrow crate labels Dec 6, 2024
@etseidl
Copy link
Contributor

etseidl commented Dec 6, 2024

I can't believe I'm saying this, but maybe 2 major releases would be more prudent 🤷

@alamb
Copy link
Contributor Author

alamb commented Dec 7, 2024

Thanks @etseidl -- I plan to leave this PR open for a while after publicizing it broadly, to allow maximal chance for comment.

I can't believe I'm saying this, but maybe 2 major releases would be more prudent 🤷

I also vacillated between 1 release and 2 releases (and almost proposed 2 releases). I updated the proposal to be 2 releases

@brancz
Copy link
Contributor

brancz commented Dec 7, 2024

I think a minimum of 1 major release, and then depending on the severity of the change (as in what is the alternative, what do behavior or changes to defaults fall into?) a judgement call is made. I think mandating 2 releases no matter what would disregard that there are various degrees of changes.

README.md Outdated Show resolved Hide resolved
README.md Outdated
#[deprecated(since = "51.0.0", note = "Use `date_part` instead")]
```

Deprecated APIs will be kept for at least two major releases after they were
Copy link
Member

Choose a reason for hiding this comment

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

The PR initially started with 1, now it's 2. This is not unimportant. The less the better from code maint. perspective

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree it is an important distinction. Let's see if any consensus emerges

@timsaucer
Copy link

timsaucer commented Dec 7, 2024

My recommendation would be 2 major releases, but @brancz makes a very good point that it should be a recommendation and not necessarily a hard rule.

@tustvold
Copy link
Contributor

tustvold commented Dec 7, 2024

Given the lag with which people upgrade to the latest major versions, we are about to cut version 54.0.0 and only 1/3 of the download base (according to crates.io)has upgraded to 53.0.0, 2 major versions is probably the right cadence.

I do wonder if we might instead use a time bound, e.g. 6 months. This would not only be less restrictive, but also translate should be look to reduce the cadence of breaking releases in future as well.

Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

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

I think that 2 major releases would be okay. Maybe we can add that for exceptional cases, after discussing (and voting?) between committers/PMCs, we can shorten/extend the time?

@alamb
Copy link
Contributor Author

alamb commented Dec 9, 2024

I think that 2 major releases would be okay. Maybe we can add that for exceptional cases, after discussing (and voting?) between committers/PMCs, we can shorten/extend the time?

Rather than trying to come up with a process now, perhaps we can just make the policy more general and say "at the discretion of the committers" or something, and we can add a more formal policy if there are specific issues caused by the informal one

@alamb
Copy link
Contributor Author

alamb commented Dec 12, 2024

I tried to incorporate all the feed back so far:

  • I change from "Deprecation Policy" to "Deprecation Guidelines"
  • Added some wording to maintain flexibility "Deprecated APIs may be removed earlier or later than these guidelines at the discretion of the maintainers."
  • Tried to clarify the example deprecation timeline wording

downstream Rust programs to still compile, but generate compiler warnings. This
gives downstream crates time to migrate prior to API removal.

All deprecated APIs are marked using the `#[deprecated]` attribute with both the
Copy link
Member

Choose a reason for hiding this comment

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

#[deprecated(since="...", note="...")] to decrease chances of creating new deprecations with missing since attr? (see #6782)

first version they were deprecated in, and what new API to use instead.

```rust
#[deprecated(since = "51.0.0", note = "Use `date_part` instead")]
Copy link
Member

Choose a reason for hiding this comment

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

like in DF deprecation guidelines, it it obvious how a contributor should fill the since field? eg is this the version string from cargo.toml, or next major from that?

Copy link
Member

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate parquet Changes to the parquet crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should we document at what rate deprecated APIs are removed?
7 participants