Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Use api_reference as the source of truth for __all__ an __dir__ #30375

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
Loading
from

Conversation

thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Nov 29, 2024

Reference Issues/PRs

Follow up to #29793

What does this implement/fix? Explain your changes.

When using auto-complete on main I see:

Screenshot 2024-11-28 at 5 42 15 PM

This includes all the objects imported in the file including modules like functools. With this PR, I get this:

Screenshot 2024-11-28 at 5 51 48 PM

Any other comments?

Using api_reference as the source of truth leads to some inconsistencies. There are items are not in api_refernece, but are in an existing __all__. These are listed in sklearn/tests/test_imports_public.py as OBJECTS_NOT_IN_API_REFERENCE. The possible causes are:

  1. A submodule (This is okay)
  2. Reimported from somewhere else, and is documented there (This is also okay, but a bit weird to have two import locations)
  3. Importable but not documented

I added some items into api_reference that seems like that should be documented in the first place.

Copy link

github-actions bot commented Nov 29, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: c68d719. Link to the linter CI: here

@adrinjalali
Copy link
Member

I just saw this, and looks pretty harmless and nice to have. Would you please resolve conflicts?

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a nit, otherwise LGTM. Not sure if we really want to cover the parts not covered in tests here.

@@ -27,7 +26,6 @@
"AdaBoostRegressor",
"BaggingClassifier",
"BaggingRegressor",
"BaseEnsemble",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O_o, should we keep this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.