Make DocBuilder migration in 44.0.0 easier

@kylebarron

Is your feature request related to a problem or challenge?

Also related to

[EPIC] A collection of items to improve DataFuson stability (reduce effort required to upgrade) #13648

While working to reproduce the issue from @kylebarron here:

Schema error when returning DenseUnion from ScalarUDF #13762

I had some compile errors because Documentation::new() has changed signature (it now has three required arguments) after this PR from @comphead

Doc gen: Attributes to support related_udf, alternative_syntax #13575

This was a bit confusing / annoying and then I had to go find what to do / how to make a section, etc

Describe the solution you'd like

Before we release 44 into the world I think we like to make the experience better / more obvious what downstream crates should do on upgrade

Describe alternatives you've considered

Alternate 1: restore `DocumentationBuilder::new` and add deprecation message

I propose renaming

DocumentationBuilder::new to something else (like new_with_section)
Add a new function named DocumentationBuilder::new or something that is `#[deprecated]``, following the deprecation guidelines to help users find the new API

The message should point them at

Using the [user_doc] proc macro (e.g. this)
Using the newly added DocumentationBuilder:new_with_section

Alternate 2: Add migration guide

Another alternate would be to start working on a 44.0.0 migration guide and include this. I think automatically telling people what to use would be bette.r

Additional context

No response

@comphead

I had some compile errors because Documentation::new() has changed signature (it now has three required arguments) after this PR from @comphead

Thanks @alamb please let me know where are the compile errors? Is it a downstream project compilation?

@alamb

what probably we can do in a short run is to have a migration document and describe migration points.
Especially it is crucial for PRs with

api_change
introducing deprecated methods
removing obsolete methods

So we need to introduce a single file doc with migration and its gonna part of review to make sure migration steps are added if any of 3 conditions above are met

WDYT @alamb

One possible scenario is have a file migration.md with following structure

Release 43.0.0
- steps to migrate feature1
- steps to migrate feature2

Release 42.0.0
- steps to migrate feature1
- steps to migrate feature2

Without the migration every migration the user never knows what to expect unless he starts to migrate

@alamb

Thanks @alamb please let me know where are the compile errors? Is it a downstream project compilation?

It was example on the issue filed by @kylebarron on #13762

Basically

static DOC: OnceLock<Documentation> = OnceLock::new();
impl ScalarUDFImpl for UnionExample {
...
    fn documentation(&self) -> Option<&Documentation> {
        Some(DOC.get_or_init(|| Documentation::builder().build().unwrap()))
    }
...
}

This compiles in 43.0.0 but on 44.0.0 you get an error about "DocumentationBuilder requires 3 arguments" or something like that. Then I followed the source and found I needed to provide &DocSection, etc....

I could have figured it out but I was focused on something else -- it would have been much nicer if it had been a warning / deprecation message that I could choose to ignore for the time being and come back to when I had time. It also would have been nice if it pointed me at what to do (aka use the nice new doc macro)

what probably we can do in a short run is to have a migration document and describe migration points. Especially it is crucial for PRs with

api_change

introducing deprecated methods

removing obsolete methods

So we need to introduce a single file doc with migration and its gonna part of review to make sure migration steps are added if any of 3 conditions above are met

WDYT @alamb

I think the file is a great idea ❤️

I think it would be great if we could pull off adding this to the reviews. I worry about being able to get contributors to write such a guide (it may be too big of a burden). Though maybe once we have some good examples it would be ok

I still believe that #[deprecatated] for a few releases is a much better pattern, FWIW as it gives downstream users more flexibility.

(I can make a PR with a proposed change, I just wanted to socialize it a bit first)

I'll create the PR soon with the sample file and some doc how to use it and how review process will be changed and we can through it

I'll create the PR soon with the sample file and some doc how to use it and how review process will be changed and we can through it

Would it be ok to do renaming functions/ deprecation (in addition)?

Here is a proposed PR:

Restore DocBuilder::new() to avoid breaking API change #13870

Navigation Menu

Repository navigation

Make `DocBuilder` migration in `44.0.0` easier #13764

Is your feature request related to a problem or challenge?

Describe the solution you'd like

Describe alternatives you've considered

Alternate 1: restore `DocumentationBuilder::new` and add deprecation message

Alternate 2: Add migration guide

Additional context

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Make DocBuilder migration in 44.0.0 easier #13764

Description

Is your feature request related to a problem or challenge?

Describe the solution you'd like

Describe alternatives you've considered

Alternate 1: restore DocumentationBuilder::new and add deprecation message

Alternate 2: Add migration guide

Additional context

Activity

comphead commented on Dec 13, 2024

comphead commented on Dec 13, 2024

comphead commented on Dec 13, 2024

alamb commented on Dec 13, 2024

alamb commented on Dec 13, 2024

comphead commented on Dec 13, 2024

alamb commented on Dec 13, 2024

alamb commented on Dec 21, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Make `DocBuilder` migration in `44.0.0` easier #13764

Alternate 1: restore `DocumentationBuilder::new` and add deprecation message