Skip to content
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

FIX-#7263: Empty docstrings should not be inherited #7264

Merged
merged 5 commits into from
May 16, 2024

Conversation

anmyachev
Copy link
Collaborator

@anmyachev anmyachev commented May 14, 2024

What do these changes do?

  • first commit message and PR title follow format outlined here

    NOTE: If you edit the PR title to match this format, you need to add another commit (even if it's empty) or amend your last commit for the CI job that checks the PR title to pick up the new PR title.

  • passes flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
  • passes black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
  • signed commit with git commit -s
  • Resolves Empty docstrings should not be inherited #7263
  • tests added and passing
  • module layout described at docs/development/architecture.rst is up-to-date

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
@anmyachev anmyachev marked this pull request as ready for review May 14, 2024 15:02
@@ -434,6 +437,7 @@ def _documentable_obj(obj: object) -> bool:
"""Check if `obj` docstring could be patched."""
return bool(
callable(obj)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Classes are also callable, but we should not modify them. An important clarification - this only applies to classes that can be stored in the attributes of the class for which we are inheriting docstrings.

@@ -239,12 +239,12 @@ def wait(self):
DaskWrapper.wait(self.list_of_blocks)


@_inherit_docstrings(PandasOnDaskDataframeVirtualPartition.__init__)
@_inherit_docstrings(PandasOnDaskDataframeVirtualPartition)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Inheriting a docstring from a method for an entire class does not seem correct.

@@ -1118,7 +1118,7 @@ def merge_asof(
tolerance=None,
allow_exact_matches: bool = True,
direction: str = "backward",
):
): # noqa: GL08
Copy link
Collaborator

Choose a reason for hiding this comment

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

why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously, an empty string was assigned and the problem was not visible.

Now the question arises: why did the checker not pay attention to the empty string? This is most likely another bug :) In this situation, the successor does not override this function (PandasQueryCompiler.merge_asof is BaseQueryCompiler.merge_asof == True), it turns out that we inherit an empty string and assign __doc_inherited__ label. And since the function is the same, we essentially disabled the check not only for the function that is docstring inherited, but also the function from which we took it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the explanation :)

@YarShev YarShev merged commit 7e40812 into modin-project:main May 16, 2024
55 checks passed
@anmyachev anmyachev deleted the issue7263 branch May 16, 2024 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Empty docstrings should not be inherited
2 participants