DOC: set __module__ on remaining top-level functions #6
Conversation
|
⏳ Code review in progress. Analyzing for code quality issues and best practices. You can monitor the review status in the checks section at the bottom of this pull request. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule: FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
![q-dev-gamma-pdx-interstellar[bot]](https://avatars.githubusercontent.com/u/225266306?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes made in this pull request are generally positive, focusing on improving API consistency by setting the __module__ attribute for several public functions in the pandas library. This change ensures that these functions are correctly associated with the 'pandas' module when accessed through the public API.
Key points:
- The main change is the addition of
__module__assignments forget_option,set_option,describe_option,reset_option, andoption_contextfunctions. - This improvement helps in maintaining a consistent and clear API structure.
A minor suggestion was made to improve the comment above these changes for better clarity. Overall, this is a valuable contribution to the pandas library, enhancing its API consistency and potentially improving the developer experience when working with these functions.
Coming soon: You'll be able to request the agent to implement changes and create commits on your pull request's source branch. This may trigger any configured CI/CD workflows automatically.
You can disable any CI/CD workflow that you have configured to run on the source branch of the pull request. For more information, see Disabling and enabling a workflow in the GitHub documentation.
| # import set_module here would cause circular import | ||
| get_option.__module__ = "pandas" | ||
| set_option.__module__ = "pandas" | ||
| describe_option.__module__ = "pandas" | ||
| reset_option.__module__ = "pandas" | ||
| option_context.__module__ = "pandas" |
There was a problem hiding this comment.
The changes made to set the __module__ attribute for several functions are a good improvement for API consistency. This ensures that these functions are correctly associated with the 'pandas' module when accessed through the public API. However, there's a minor issue with the comment above these changes.
| # import set_module here would cause circular import | |
| get_option.__module__ = "pandas" | |
| set_option.__module__ = "pandas" | |
| describe_option.__module__ = "pandas" | |
| reset_option.__module__ = "pandas" | |
| option_context.__module__ = "pandas" | |
| # Set the __module__ attribute for public API functions | |
| get_option.__module__ = "pandas" | |
| set_option.__module__ = "pandas" | |
| describe_option.__module__ = "pandas" | |
| reset_option.__module__ = "pandas" | |
| option_context.__module__ = "pandas" |
|
⏳ Code review in progress. Analyzing for code quality issues and best practices. You can monitor the review status in the checks section at the bottom of this pull request. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule: FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
lishaxia
left a comment
lishaxia
left a comment
There was a problem hiding this comment.
This PR adds __module__ to remaining top-level functions for API consistency. The changes look good overall with a few minor issues to address:
-
In
pandas/tests/api/test_api.py, the import check for objects should only run if there are no failures in the initial module check, otherwise it might fail prematurely. -
In
pandas/conftest.py, thempl_cleanupfixture now has an unuseddoctest_namespaceparameter that should be removed. -
In
pandas/_config/config.py, the comment about circular imports could be more specific about which module would cause the circular import.
The PR successfully implements module attribution for all the relevant functions, which will make the API more consistent. After addressing these minor issues, the PR is ready for merging.
Coming soon: You'll be able to request the agent to implement changes and create commits on your pull request's source branch. This may trigger any configured CI/CD workflows automatically.
You can disable any CI/CD workflow that you have configured to run on the source branch of the pull request. For more information, see Disabling and enabling a workflow in the GitHub documentation.
| @pytest.fixture(autouse=True) | ||
| def mpl_cleanup(doctest_namespace): |
There was a problem hiding this comment.
The mpl_cleanup fixture now includes doctest_namespace parameter but doesn't actually use it. This parameter should either be used or removed to avoid confusion.
| @pytest.fixture(autouse=True) | |
| def mpl_cleanup(doctest_namespace): | |
| @pytest.fixture(autouse=True) | |
| def mpl_cleanup(): |
| def test_set_module(): | ||
| assert pd.DataFrame.__module__ == "pandas" | ||
| assert pd.CategoricalDtype.__module__ == "pandas" | ||
| assert pd.DatetimeTZDtype.__module__ == "pandas" | ||
| assert pd.PeriodDtype.__module__ == "pandas" | ||
| assert pd.IntervalDtype.__module__ == "pandas" | ||
| assert pd.SparseDtype.__module__ == "pandas" | ||
| assert pd.ArrowDtype.__module__ == "pandas" | ||
| assert pd.StringDtype.__module__ == "pandas" | ||
| assert pd.BooleanDtype.__module__ == "pandas" | ||
| assert pd.Int8Dtype.__module__ == "pandas" | ||
| assert pd.Int16Dtype.__module__ == "pandas" | ||
| assert pd.Int32Dtype.__module__ == "pandas" | ||
| assert pd.Int64Dtype.__module__ == "pandas" | ||
| assert pd.UInt8Dtype.__module__ == "pandas" | ||
| assert pd.UInt16Dtype.__module__ == "pandas" | ||
| assert pd.UInt32Dtype.__module__ == "pandas" | ||
| assert pd.UInt64Dtype.__module__ == "pandas" | ||
| assert pd.Float32Dtype.__module__ == "pandas" | ||
| assert pd.Float64Dtype.__module__ == "pandas" | ||
| assert pd.Index.__module__ == "pandas" | ||
| assert pd.CategoricalIndex.__module__ == "pandas" | ||
| assert pd.DatetimeIndex.__module__ == "pandas" | ||
| assert pd.IntervalIndex.__module__ == "pandas" | ||
| assert pd.MultiIndex.__module__ == "pandas" | ||
| assert pd.PeriodIndex.__module__ == "pandas" | ||
| assert pd.RangeIndex.__module__ == "pandas" | ||
| assert pd.TimedeltaIndex.__module__ == "pandas" | ||
| assert pd.Period.__module__ == "pandas" | ||
| assert pd.Timestamp.__module__ == "pandas" | ||
| assert pd.Timedelta.__module__ == "pandas" | ||
| assert pd.concat.__module__ == "pandas" | ||
| assert pd.isna.__module__ == "pandas" | ||
| assert pd.notna.__module__ == "pandas" | ||
| assert pd.merge.__module__ == "pandas" | ||
| assert pd.merge_ordered.__module__ == "pandas" | ||
| assert pd.merge_asof.__module__ == "pandas" | ||
| assert pd.read_csv.__module__ == "pandas" | ||
| assert pd.read_table.__module__ == "pandas" | ||
| assert pd.read_fwf.__module__ == "pandas" | ||
| assert pd.Series.__module__ == "pandas" | ||
| assert pd.date_range.__module__ == "pandas" | ||
| assert pd.bdate_range.__module__ == "pandas" | ||
| assert pd.period_range.__module__ == "pandas" | ||
| assert pd.timedelta_range.__module__ == "pandas" | ||
| assert pd.to_datetime.__module__ == "pandas" | ||
| assert pd.to_timedelta.__module__ == "pandas" | ||
| assert pd.to_numeric.__module__ == "pandas" | ||
| assert pd.NamedAgg.__module__ == "pandas" | ||
| assert pd.IndexSlice.__module__ == "pandas" | ||
| assert pd.lreshape.__module__ == "pandas" | ||
| assert pd.melt.__module__ == "pandas" | ||
| assert pd.wide_to_long.__module__ == "pandas" | ||
| assert pd.crosstab.__module__ == "pandas" | ||
| assert pd.pivot_table.__module__ == "pandas" | ||
| assert pd.pivot.__module__ == "pandas" | ||
| assert pd.cut.__module__ == "pandas" | ||
| assert pd.qcut.__module__ == "pandas" | ||
| assert pd.read_clipboard.__module__ == "pandas" | ||
| assert pd.ExcelFile.__module__ == "pandas" | ||
| assert pd.ExcelWriter.__module__ == "pandas" | ||
| assert pd.read_excel.__module__ == "pandas" | ||
| assert pd.read_feather.__module__ == "pandas" | ||
| assert pd.set_eng_float_format.__module__ == "pandas" | ||
| assert pd.read_html.__module__ == "pandas" | ||
| assert pd.read_iceberg.__module__ == "pandas" | ||
| assert pd.read_json.__module__ == "pandas" | ||
| assert pd.json_normalize.__module__ == "pandas" | ||
| assert pd.read_orc.__module__ == "pandas" | ||
| assert pd.read_parquet.__module__ == "pandas" | ||
| assert pd.read_pickle.__module__ == "pandas" | ||
| assert pd.to_pickle.__module__ == "pandas" | ||
| assert pd.HDFStore.__module__ == "pandas" | ||
| assert pd.read_hdf.__module__ == "pandas" | ||
| assert pd.read_sas.__module__ == "pandas" | ||
| assert pd.read_spss.__module__ == "pandas" | ||
| assert pd.read_sql.__module__ == "pandas" | ||
| assert pd.read_sql_query.__module__ == "pandas" | ||
| assert pd.read_sql_table.__module__ == "pandas" | ||
| assert pd.read_stata.__module__ == "pandas" | ||
| assert pd.read_xml.__module__ == "pandas" | ||
| assert api.typing.SeriesGroupBy.__module__ == "pandas.api.typing" | ||
| assert api.typing.DataFrameGroupBy.__module__ == "pandas.api.typing" |
There was a problem hiding this comment.
The removed test_set_module function contains explicit checks for module attributes that are now redundant with the enhanced test_attributes_module function that verifies the same property more comprehensively.
| if inspect.isfunction(obj) or type(obj).__name__ == "cython_function_or_method": | ||
| # We have not set __module__ on public functions; may do | ||
| # so in the future. | ||
| continue |
There was a problem hiding this comment.
The removal of this condition means functions will now also be checked for proper module attributes. This is appropriate since the PR is adding module to functions as well.
| # Check that all objects can indeed be imported from their __module__ | ||
| failures = [] | ||
| for module_name, name, obj in objs: | ||
| module = importlib.import_module(obj.__module__) | ||
| try: | ||
| getattr(module, name) | ||
| except Exception: | ||
| failures.append((module_name, name, type(obj), obj.__module__)) | ||
| assert len(failures) == 0, "\n".join(str(e) for e in failures) |
There was a problem hiding this comment.
🛑 Logic Error: This additional test will fail if an object's module is set correctly but the object can't be imported from that module. The check should happen after verifying that all objects have correct module values.
| # Check that all objects can indeed be imported from their __module__ | |
| failures = [] | |
| for module_name, name, obj in objs: | |
| module = importlib.import_module(obj.__module__) | |
| try: | |
| getattr(module, name) | |
| except Exception: | |
| failures.append((module_name, name, type(obj), obj.__module__)) | |
| assert len(failures) == 0, "\n".join(str(e) for e in failures) | |
| # Check that all objects can indeed be imported from their __module__ | |
| # Only run this check after confirming no failures in the __module__ check | |
| if len(failures) == 0: | |
| import_failures = [] | |
| for module_name, name, obj in objs: | |
| module = importlib.import_module(obj.__module__) | |
| try: | |
| getattr(module, name) | |
| except Exception: | |
| import_failures.append((module_name, name, type(obj), obj.__module__)) | |
| assert len(import_failures) == 0, "\n".join(str(e) for e in import_failures) |
| return True | ||
|
|
||
|
|
||
| # import set_module here would cause circular import |
There was a problem hiding this comment.
The comment "import set_module here would cause circular import" should provide more context. A better explanation would indicate which module it would create a circular import with.
| # import set_module here would cause circular import | |
| # Importing set_module from pandas.util._decorators would cause circular import |
|
⏳ Code review in progress. Analyzing for code quality issues and best practices. You can monitor the review status in the checks section at the bottom of this pull request. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule: FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
lishaxia
left a comment
lishaxia
left a comment
There was a problem hiding this comment.
This PR successfully implements consistent __module__ attribute setting for remaining top-level functions in pandas. The changes follow a clear pattern:
✅ Strengths:
- Consistent approach: Uses
@set_moduledecorator consistently across files to set appropriate public API paths - Improved test infrastructure: Replaces manual test assertions with automated verification that scales better
- Proper handling of circular imports: Uses manual
__module__assignment in_config/config.pywhere decorators would cause circular imports - Comprehensive coverage: Covers functions across multiple modules (api.types, api.extensions, plotting, etc.)
⚠️ Areas for attention:
- conftest.py change: The addition of
autouse=Trueto thempl_cleanupfixture needs verification as it changes test behavior significantly - Test coverage: The new
test_attributes_modulefunction provides better coverage than the removed manual test
📋 Summary:
The implementation correctly addresses the goal of setting __module__ on remaining top-level functions for API consistency. The approach is sound and the test improvements are valuable. Just need to confirm the conftest.py change is intentional.
Overall assessment: Good implementation with one minor clarification needed.
Coming soon: You'll be able to request the agent to implement changes and create commits on your pull request's source branch. This may trigger any configured CI/CD workflows automatically.
You can disable any CI/CD workflow that you have configured to run on the source branch of the pull request. For more information, see Disabling and enabling a workflow in the GitHub documentation.
| @pytest.fixture(autouse=True) | ||
| def mpl_cleanup(doctest_namespace): |
There was a problem hiding this comment.
The autouse=True parameter addition changes the behavior of this fixture significantly. This fixture will now automatically run for every test, which may not be intended. The doctest_namespace parameter also suggests this might be related to doctest functionality.
Please verify this change is intentional and document why autouse=True is needed here.
|
⏳ Code review in progress. Analyzing for code quality issues and best practices. You can monitor the review status in the checks section at the bottom of this pull request. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule: FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
lishaxia
left a comment
lishaxia
left a comment
There was a problem hiding this comment.
This PR successfully implements the goal of setting __module__ attributes on remaining top-level functions for API consistency. The changes are well-structured and comprehensive:
Positive aspects:
- Systematic addition of
@set_moduledecorators across multiple modules to ensure functions have the correct public API module path - Replacement of the manual
test_set_module()function with a more generic and maintainabletest_attributes_module()test - Removal of function exclusions from module attribute checking, indicating functions now properly have their
__module__set - Enhanced test coverage with additional validation that objects can be imported from their declared
__module__
Areas of concern:
- The change to
pandas/conftest.pyappears unrelated to the PR's main objective and should be addressed
The implementation follows pandas' established patterns for module attribution and will improve API consistency for users inspecting function origins.
Coming soon: You'll be able to request the agent to implement changes and create commits on your pull request's source branch. This may trigger any configured CI/CD workflows automatically.
You can disable any CI/CD workflow that you have configured to run on the source branch of the pull request. For more information, see Disabling and enabling a workflow in the GitHub documentation.
| @pytest.fixture(autouse=True) | ||
| def mpl_cleanup(doctest_namespace): |
There was a problem hiding this comment.
This change appears unrelated to setting __module__ attributes. The modification adds autouse=True and a doctest_namespace parameter to the mpl_cleanup fixture, which seems to be a separate concern from the PR's stated goal of setting __module__ on remaining top-level functions.
|
⏳ Code review in progress. Analyzing for code quality issues and best practices. You can monitor the review status in the checks section at the bottom of this pull request. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule: FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
lishaxia
left a comment
lishaxia
left a comment
There was a problem hiding this comment.
This PR successfully completes the work of setting __module__ attributes on remaining top-level functions for API consistency. The changes are well-structured and comprehensive:
Key Improvements:
- ✅ Adds
@set_moduledecorators to functions across multiple modules to set proper__module__attributes - ✅ Uses appropriate public API paths (e.g.,
pandas.api.types,pandas.api.extensions,pandas.plotting) - ✅ Handles circular import issues gracefully with manual assignment for config functions
- ✅ Replaces manual test with automated discovery-based validation
- ✅ Adds additional validation to ensure objects are actually importable from their declared modules
Technical Quality:
- The decorator usage is consistent and follows pandas conventions
- Import organization is clean and logical
- The test improvements make the validation more robust and maintainable
- Circular import handling is documented and appropriate
Impact:
This change improves API consistency by ensuring all public functions have their __module__ attribute set to the public import path, which benefits introspection tools, documentation generation, and user experience.
The implementation is solid and ready for merge.
Coming soon: You'll be able to request the agent to implement changes and create commits on your pull request's source branch. This may trigger any configured CI/CD workflows automatically.
You can disable any CI/CD workflow that you have configured to run on the source branch of the pull request. For more information, see Disabling and enabling a workflow in the GitHub documentation.
| # import set_module here would cause circular import | ||
| get_option.__module__ = "pandas" | ||
| set_option.__module__ = "pandas" | ||
| describe_option.__module__ = "pandas" | ||
| reset_option.__module__ = "pandas" | ||
| option_context.__module__ = "pandas" |
There was a problem hiding this comment.
The manual assignment of __module__ attributes at the end of the file is a reasonable approach to avoid circular imports. However, consider adding a brief comment explaining why these specific functions need manual assignment rather than using the @set_module decorator.
| # import set_module here would cause circular import | |
| get_option.__module__ = "pandas" | |
| set_option.__module__ = "pandas" | |
| describe_option.__module__ = "pandas" | |
| reset_option.__module__ = "pandas" | |
| option_context.__module__ = "pandas" | |
| # import set_module here would cause circular import | |
| # Manual assignment needed to avoid circular dependency with pandas.util._decorators | |
| get_option.__module__ = "pandas" | |
| set_option.__module__ = "pandas" | |
| describe_option.__module__ = "pandas" | |
| reset_option.__module__ = "pandas" | |
| option_context.__module__ = "pandas" |
| def test_set_module(): | ||
| assert pd.DataFrame.__module__ == "pandas" | ||
| assert pd.CategoricalDtype.__module__ == "pandas" | ||
| assert pd.DatetimeTZDtype.__module__ == "pandas" | ||
| assert pd.PeriodDtype.__module__ == "pandas" | ||
| assert pd.IntervalDtype.__module__ == "pandas" | ||
| assert pd.SparseDtype.__module__ == "pandas" | ||
| assert pd.ArrowDtype.__module__ == "pandas" | ||
| assert pd.StringDtype.__module__ == "pandas" | ||
| assert pd.BooleanDtype.__module__ == "pandas" | ||
| assert pd.Int8Dtype.__module__ == "pandas" | ||
| assert pd.Int16Dtype.__module__ == "pandas" | ||
| assert pd.Int32Dtype.__module__ == "pandas" | ||
| assert pd.Int64Dtype.__module__ == "pandas" | ||
| assert pd.UInt8Dtype.__module__ == "pandas" | ||
| assert pd.UInt16Dtype.__module__ == "pandas" | ||
| assert pd.UInt32Dtype.__module__ == "pandas" | ||
| assert pd.UInt64Dtype.__module__ == "pandas" | ||
| assert pd.Float32Dtype.__module__ == "pandas" | ||
| assert pd.Float64Dtype.__module__ == "pandas" | ||
| assert pd.Index.__module__ == "pandas" | ||
| assert pd.CategoricalIndex.__module__ == "pandas" | ||
| assert pd.DatetimeIndex.__module__ == "pandas" | ||
| assert pd.IntervalIndex.__module__ == "pandas" | ||
| assert pd.MultiIndex.__module__ == "pandas" | ||
| assert pd.PeriodIndex.__module__ == "pandas" | ||
| assert pd.RangeIndex.__module__ == "pandas" | ||
| assert pd.TimedeltaIndex.__module__ == "pandas" | ||
| assert pd.Period.__module__ == "pandas" | ||
| assert pd.Timestamp.__module__ == "pandas" | ||
| assert pd.Timedelta.__module__ == "pandas" | ||
| assert pd.concat.__module__ == "pandas" | ||
| assert pd.isna.__module__ == "pandas" | ||
| assert pd.notna.__module__ == "pandas" | ||
| assert pd.merge.__module__ == "pandas" | ||
| assert pd.merge_ordered.__module__ == "pandas" | ||
| assert pd.merge_asof.__module__ == "pandas" | ||
| assert pd.read_csv.__module__ == "pandas" | ||
| assert pd.read_table.__module__ == "pandas" | ||
| assert pd.read_fwf.__module__ == "pandas" | ||
| assert pd.Series.__module__ == "pandas" | ||
| assert pd.date_range.__module__ == "pandas" | ||
| assert pd.bdate_range.__module__ == "pandas" | ||
| assert pd.period_range.__module__ == "pandas" | ||
| assert pd.timedelta_range.__module__ == "pandas" | ||
| assert pd.to_datetime.__module__ == "pandas" | ||
| assert pd.to_timedelta.__module__ == "pandas" | ||
| assert pd.to_numeric.__module__ == "pandas" | ||
| assert pd.NamedAgg.__module__ == "pandas" | ||
| assert pd.IndexSlice.__module__ == "pandas" | ||
| assert pd.lreshape.__module__ == "pandas" | ||
| assert pd.melt.__module__ == "pandas" | ||
| assert pd.wide_to_long.__module__ == "pandas" | ||
| assert pd.crosstab.__module__ == "pandas" | ||
| assert pd.pivot_table.__module__ == "pandas" | ||
| assert pd.pivot.__module__ == "pandas" | ||
| assert pd.cut.__module__ == "pandas" | ||
| assert pd.qcut.__module__ == "pandas" | ||
| assert pd.read_clipboard.__module__ == "pandas" | ||
| assert pd.ExcelFile.__module__ == "pandas" | ||
| assert pd.ExcelWriter.__module__ == "pandas" | ||
| assert pd.read_excel.__module__ == "pandas" | ||
| assert pd.read_feather.__module__ == "pandas" | ||
| assert pd.set_eng_float_format.__module__ == "pandas" | ||
| assert pd.read_html.__module__ == "pandas" | ||
| assert pd.read_iceberg.__module__ == "pandas" | ||
| assert pd.read_json.__module__ == "pandas" | ||
| assert pd.json_normalize.__module__ == "pandas" | ||
| assert pd.read_orc.__module__ == "pandas" | ||
| assert pd.read_parquet.__module__ == "pandas" | ||
| assert pd.read_pickle.__module__ == "pandas" | ||
| assert pd.to_pickle.__module__ == "pandas" | ||
| assert pd.HDFStore.__module__ == "pandas" | ||
| assert pd.read_hdf.__module__ == "pandas" | ||
| assert pd.read_sas.__module__ == "pandas" | ||
| assert pd.read_spss.__module__ == "pandas" | ||
| assert pd.read_sql.__module__ == "pandas" | ||
| assert pd.read_sql_query.__module__ == "pandas" | ||
| assert pd.read_sql_table.__module__ == "pandas" | ||
| assert pd.read_stata.__module__ == "pandas" | ||
| assert pd.read_xml.__module__ == "pandas" | ||
| assert api.typing.SeriesGroupBy.__module__ == "pandas.api.typing" | ||
| assert api.typing.DataFrameGroupBy.__module__ == "pandas.api.typing" | ||
|
|
There was a problem hiding this comment.
The removal of the test_set_module function is appropriate since it was replaced by the more comprehensive test_attributes_module function. The old test was manually checking specific functions, while the new approach automatically discovers and validates all pandas objects.
| # Check that all objects can indeed be imported from their __module__ | ||
| failures = [] | ||
| for module_name, name, obj in objs: | ||
| module = importlib.import_module(obj.__module__) | ||
| try: | ||
| getattr(module, name) | ||
| except Exception: | ||
| failures.append((module_name, name, type(obj), obj.__module__)) | ||
| assert len(failures) == 0, "\n".join(str(e) for e in failures) |
There was a problem hiding this comment.
Good addition! This second validation step ensures that objects can actually be imported from their declared __module__, which catches cases where the __module__ attribute is set incorrectly but the object isn't actually available at that import path.
| if inspect.isfunction(obj) or type(obj).__name__ == "cython_function_or_method": | ||
| # We have not set __module__ on public functions; may do | ||
| # so in the future. | ||
| continue |
There was a problem hiding this comment.
The removal of this function exclusion logic is correct since functions now have their __module__ attributes properly set and should be included in the validation.
| from pandas._config import using_string_dtype | ||
|
|
||
| from pandas._libs.missing import check_na_tuples_nonequal | ||
| from pandas.util._decorators import set_module |
There was a problem hiding this comment.
Good addition of the set_module import. The placement after other imports is appropriate and follows the existing import organization.
|
⏳ Code review in progress. Analyzing for code quality issues and best practices. You can monitor the review status in the checks section at the bottom of this pull request. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule: FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
lishaxia
left a comment
lishaxia
left a comment
There was a problem hiding this comment.
This PR successfully implements the goal of setting __module__ attributes on remaining top-level functions for API consistency. The changes are well-structured and comprehensive:
Positive aspects:
- Systematic addition of
@set_moduledecorators to functions across multiple modules - Proper module path assignments (e.g., "pandas.api.types", "pandas.api.extensions", "pandas")
- Manual
__module__assignment for config functions to avoid circular imports - Updated test infrastructure to validate the module assignments
- Removal of outdated hardcoded test assertions in favor of a more robust testing approach
Areas of concern:
- The change to
mpl_cleanupfixture inconftest.pyappears unrelated to the main PR objective and should be addressed separately
The implementation correctly addresses the API consistency issue by ensuring all public functions have their __module__ attribute set to the appropriate public import path, which will improve the user experience when inspecting pandas objects.
Coming soon: You'll be able to request the agent to implement changes and create commits on your pull request's source branch. This may trigger any configured CI/CD workflows automatically.
You can disable any CI/CD workflow that you have configured to run on the source branch of the pull request. For more information, see Disabling and enabling a workflow in the GitHub documentation.
| @pytest.fixture(autouse=True) | ||
| def mpl_cleanup(doctest_namespace): |
There was a problem hiding this comment.
This change appears unrelated to the PR's main purpose of setting __module__ attributes. The addition of autouse=True and the doctest_namespace parameter changes the behavior of the mpl_cleanup fixture significantly. This should be in a separate PR with proper justification for why this change is needed.
|
⏳ Code review in progress. Analyzing for code quality issues and best practices. You can monitor the review status in the checks section at the bottom of this pull request. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule: FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
lishaxia
left a comment
lishaxia
left a comment
There was a problem hiding this comment.
This PR systematically adds @set_module decorators to remaining top-level functions across the pandas codebase to ensure API consistency. The changes are well-structured and follow a consistent pattern.
Key improvements:
- Adds
@set_moduledecorators to functions in core modules to set their__module__attribute to the appropriate public import path - Updates the test framework to use a more generic and comprehensive approach for validating module attributes
- Removes hardcoded function exclusions from module checking, indicating these functions now properly have their
__module__set
Files reviewed:
- Core pandas modules (
_config/config.py,_libs/lib.pyx,core/,plotting/, etc.) - Test infrastructure updates (
tests/api/test_api.py)
The implementation is consistent across all modified files and the test updates appropriately validate that objects can be imported from their declared __module__. This change enhances the pandas API's consistency and introspection capabilities.
Overall assessment: ✅ Approved - The changes are systematic, well-implemented, and improve API consistency without introducing any apparent issues.
Coming soon: You'll be able to request the agent to implement changes and create commits on your pull request's source branch. This may trigger any configured CI/CD workflows automatically.
You can disable any CI/CD workflow that you have configured to run on the source branch of the pull request. For more information, see Disabling and enabling a workflow in the GitHub documentation.
|
⏳ Code review in progress. Analyzing for code quality issues and best practices. You can monitor the review status in the checks section at the bottom of this pull request. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule: FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
lishaxia
left a comment
lishaxia
left a comment
There was a problem hiding this comment.
This PR successfully implements the goal of setting __module__ attributes on remaining top-level functions for API consistency. The changes are well-structured and comprehensive:
Key Improvements:
- Consistently applies
@set_module()decorators to functions across multiple modules - Properly handles circular import issues in config functions by manually setting
__module__ - Updates the test framework to be more robust and comprehensive
- Ensures all public functions appear to come from their intended public API modules
Technical Quality:
- The decorator usage is consistent and follows established patterns
- The circular import workaround in
pandas/_config/config.pyis appropriate - The test improvements make verification more thorough by checking both module attribution and importability
The implementation correctly addresses the API consistency goal without introducing any apparent issues. The changes will improve the user experience by ensuring that help() and introspection tools show the correct module paths for pandas functions.
Coming soon: You'll be able to request the agent to implement changes and create commits on your pull request's source branch. This may trigger any configured CI/CD workflows automatically.
You can disable any CI/CD workflow that you have configured to run on the source branch of the pull request. For more information, see Disabling and enabling a workflow in the GitHub documentation.
|
⏳ Code review in progress. Analyzing for code quality issues and best practices. You can monitor the review status in the checks section at the bottom of this pull request. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule: FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
lishaxia
left a comment
lishaxia
left a comment
There was a problem hiding this comment.
This PR successfully implements __module__ attribute setting on remaining top-level functions for API consistency. The changes are well-structured and comprehensive:
Positive aspects:
- Systematic application of
@set_module()decorators across different pandas modules - Proper handling of circular import issues by manually setting
__module__attributes where needed - Enhanced test coverage that not only checks
__module__attributes but also verifies functions can be imported from their declared modules - Removal of hardcoded test assertions in favor of a more robust, general testing approach
Key improvements:
- Functions now correctly appear to come from their public API modules (e.g.,
pandas.api.types,pandas.plotting) rather than internal implementation modules - Better API consistency for end users who inspect function modules
- More maintainable test infrastructure that will catch future inconsistencies
Minor concern:
- The change to
pandas/conftest.pyappears unrelated to the main purpose of this PR and should ideally be in a separate commit/PR
Overall, this is a solid improvement to pandas' API consistency that will benefit users who rely on introspection of pandas objects.
Coming soon: You'll be able to request the agent to implement changes and create commits on your pull request's source branch. This may trigger any configured CI/CD workflows automatically.
You can disable any CI/CD workflow that you have configured to run on the source branch of the pull request. For more information, see Disabling and enabling a workflow in the GitHub documentation.
| @pytest.fixture(autouse=True) | ||
| def mpl_cleanup(doctest_namespace): |
There was a problem hiding this comment.
This change appears unrelated to setting __module__ attributes on functions. The modification adds autouse=True and a doctest_namespace parameter to the mpl_cleanup fixture. This seems like it should be in a separate PR focused on test infrastructure changes rather than mixed with the API consistency improvements.
Sets module attribute on remaining top-level functions for API consistency.
Co-authored-by: Richard Shadrach [email protected]