Repository navigation
Make DocBuilder migration in 44.0.0 easier #13764
Description
Activity
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 withapi_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.mdwith following structureRelease 43.0.0 - steps to migrate feature1 - steps to migrate feature2 Release 42.0.0 - steps to migrate feature1 - steps to migrate feature2Without the migration every migration the user never knows what to expect unless he starts to migrate
Reacted by Bruce RitchieThanks @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.Reacted by Kyle Barron and Oleks V(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:
Is your feature request related to a problem or challenge?
Also related to
While working to reproduce the issue from @kylebarron here:
I had some compile errors because
Documentation::new()has changed signature (it now has three required arguments) after this PR from @compheadrelated_udf,alternative_syntax#13575This 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::newand add deprecation messageI propose renaming
DocumentationBuilder::newto something else (likenew_with_section)DocumentationBuilder::newor something that is `#[deprecated]``, following the deprecation guidelines to help users find the new APIThe message should point them at
user_doc] proc macro (e.g. this)DocumentationBuilder:new_with_sectionAlternate 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