You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2024/12/13 18:11:59 UTC
[I] Make `DocBuilder` migration in `44.0.0` easier [datafusion]
alamb opened a new issue, #13764:
URL: https://github.com/apache/datafusion/issues/13764
### Is your feature request related to a problem or challenge?
Also related to
- https://github.com/apache/datafusion/issues/13648
While working to reproduce the issue from @kylebarron here:
- https://github.com/apache/datafusion/issues/13762
I had some compile errors because `Documentation::new()` has changed signature (it now has three required arguments) after this PR from @comphead
- https://github.com/apache/datafusion/pull/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
1. `DocumentationBuilder::new` to something else (like `new_with_section`)
2. Add a new function named `DocumentationBuilder::new` or something that is `#[deprecated]``, following the [deprecation guidelines](https://github.com/apache/datafusion/blob/main/README.md) to help users find the new API
The message should point them at
1. Using the [`user_doc`] proc macro (e.g. [this](https://github.com/apache/datafusion/blob/03e39da62e403e064d21b57e9d6c200464c03749/datafusion/macros/src/user_doc.rs#L23))
2. 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_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Make `DocBuilder` migration in `44.0.0` easier [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #13764:
URL: https://github.com/apache/datafusion/issues/13764#issuecomment-2557975306
Here is a proposed PR:
- https://github.com/apache/datafusion/pull/13870
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Make `DocBuilder` migration in `44.0.0` easier [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #13764:
URL: https://github.com/apache/datafusion/issues/13764#issuecomment-2542120858
> 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 https://github.com/apache/datafusion/issues/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Make `DocBuilder` migration in `44.0.0` easier [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #13764:
URL: https://github.com/apache/datafusion/issues/13764#issuecomment-2542465790
> 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)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Make `DocBuilder` migration in `44.0.0` easier [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed issue #13764: Make `DocBuilder` migration in `44.0.0` easier
URL: https://github.com/apache/datafusion/issues/13764
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Make `DocBuilder` migration in `44.0.0` easier [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on issue #13764:
URL: https://github.com/apache/datafusion/issues/13764#issuecomment-2542021552
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Make `DocBuilder` migration in `44.0.0` easier [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on issue #13764:
URL: https://github.com/apache/datafusion/issues/13764#issuecomment-2542010941
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Make `DocBuilder` migration in `44.0.0` easier [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on issue #13764:
URL: https://github.com/apache/datafusion/issues/13764#issuecomment-2542387505
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Make `DocBuilder` migration in `44.0.0` easier [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #13764:
URL: https://github.com/apache/datafusion/issues/13764#issuecomment-2542259505
(I can make a PR with a proposed change, I just wanted to socialize it a bit first)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Make `DocBuilder` migration in `44.0.0` easier [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on issue #13764:
URL: https://github.com/apache/datafusion/issues/13764#issuecomment-2542024565
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
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org