docs: Document the convention for internal public APIs - #25495
2010YOUY01 wants to merge 3 commits into
Conversation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #25495 +/- ##
==========================================
+ Coverage 82.37% 82.41% +0.04%
==========================================
Files 1138 1138
Lines 433506 435429 +1923
Branches 433506 435429 +1923
==========================================
+ Hits 357102 358877 +1775
+ Misses 54850 54848 -2
- Partials 21554 21704 +150 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
alamb
left a comment
There was a problem hiding this comment.
Thanks @2010YOUY01 and @adriangb -- this makes sense to me
I would personally suggest we try and use a feature for this method rather than not documenting certain methods, but I think this is already an improvement in my mind
|
|
||
| ### DataFusion-internal Public APIs | ||
|
|
||
| Some APIs require `pub` visibility for use across DataFusion crates. Examples |
There was a problem hiding this comment.
I recommend we also document "why" we have such APIs when so much of DataFusion's flexibility is predicated on extensible APIs.
Maybe the idea is that these APIs are really meant as internal implementation details, but for some logistical reason they have to be in different crates. Thus the "internal" APIs aren't intended as extension points, and may change / etc without warning?
| Some APIs require `pub` visibility for use across DataFusion crates. Examples | ||
| include: | ||
|
|
||
| 1. Test helpers. |
There was a problem hiding this comment.
I think in general multiple types of APIs just makes DataFUsion more complicated and I would like to suggest we minimize the number of such APIs as much as possible
For example, for test helpers, we could potentially use a feature (test_helpers for example) to mark that they are not part of the public API rather than marking them as a doc
| } | ||
| ``` | ||
|
|
||
| These APIs are hidden from generated documentation and are not considered part |
There was a problem hiding this comment.
would recommend putting this detail in the intro
Which issue does this PR close?
Rationale for this change
The purpose should be clear from the PR diff itself.
It is an existing convention, this PR document it in API policy.
There are other ~5 places have this pattern, I plan to make a follow-up PR to update their comments.
What changes are included in this PR?
What is the testing strategy for this PR?
Are there any user-facing changes?