-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[test] Add test for exposed top-level APIs #4361
Conversation
✔️ Deploy Preview for docsite-preview ready! 🔨 Explore the source changes: 7312ea0 🔍 Inspect the deploy log: https://app.netlify.com/sites/docsite-preview/deploys/6215efa547aa970007dfe348 😎 Browse the preview: https://deploy-preview-4361--docsite-preview.netlify.app |
/format |
/format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I do wonder if this is the best way to control API. Have you found any reference for other Python projects doing similar implementation? I'm a bit concerned about the maintanence cost...
Most Python projects don't control API. Super projects like numpy and torch put much more efforts on the doc site - manually selecting APIs to write and show, and not responsible for anything not on the doc site. We are currently taking an fully automatic approach because we currently don't have so many people to maintain the doc site. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome thanks!
def test_api(src): | ||
# When Python version is below 3.7, deprecated names are | ||
# handled as normal names, which will fail this test. | ||
assert sys.version_info < (3, 7) or [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need a better error message to instruct developers how to tell if their apis should be added or just hidden, and how to update the lists if they really want to expose ;)
But this can be a followup for sure!
Related issue = #3782
Since we have put great efforts on picking user APIs, we need a systematic way to prevent future developers from accidentally breaking the API list. This PR aims at solving this problem by testing exposed top-level APIs (including sub-APIs for top-level classes).