Open h-mayorquin opened 3 weeks ago
h-mayorquin
commented
1 week ago ne point of discussion is should we include Returns sections for standard methods like get_metadata, get_metadata_schema, etc.? Or should we just have them be 1-liners.
I am fine with adding it, but this is just a minimal effort to get the ruff passing and then we can improve piecewise and unify.
bendichter
commented
5 days ago When a method in a child class is overriding a parent class and does not have a docstring, it takes on the docstring of the parent. I think this works fine and in many cases makes it unnecessary to create a new docstring. However, it looks like this ruff inspector does not respect this and requires docstrings to be defined even on child classes where the parent has an appropriate docstring defined. Is that correct? If that's the case, I think we should be able to write a custom check that will work better for us.
bendichter
commented
5 days ago e.g.
import importlib
import inspect
import pkgutil
def check_package_docstrings(package_name):
"""
Check if all public methods of public classes in public modules of a package have docstrings.
Args:
package_name (str): The name of the package to check.
Returns:
dict: A dictionary containing information about missing docstrings.
"""
missing_docstrings = {}
package = importlib.import_module(package_name)
for _, module_name, _ in pkgutil.walk_packages(package.__path__, package.__name__ + '.'):
try:
module = importlib.import_module(module_name)
for name, obj in inspect.getmembers(module):
if inspect.isclass(obj) and not name.startswith('_'):
class_missing = check_class_docstrings(obj)
if class_missing:
missing_docstrings[f"{module_name}.{name}"] = class_missing
except ImportError as e:
print(f"Error importing module {module_name}: {e}")
return missing_docstrings
def check_class_docstrings(cls):
"""
Check if all public methods of a class have docstrings.
Args:
cls (type): The class to check.
Returns:
list: A list of method names without docstrings.
"""
missing = []
for name, method in inspect.getmembers(cls, inspect.isfunction):
if not name.startswith('_') and not method.__doc__:
missing.append(name)
return missing
# Example usage
if __name__ == "__main__":
package_name = "your_package_name"
result = check_package_docstrings(package_name)
if result:
print("Missing docstrings found:")
for class_name, methods in result.items():
print(f" {class_name}: {', '.join(methods)}")
else:
print("All public methods have docstrings.")When a method in a child class is overriding a parent class and does not have a docstring, it takes on the docstring of the parent. I think this works fine and in many cases makes it unnecessary to create a new docstring. However, it looks like this ruff inspector does not respect this and requires docstrings to be defined even on child classes where the parent has an appropriate docstring defined. Is that correct? If that's the case, I think we should be able to write a custom check that will work better for us.
Yeah, that's a downside. The approach that I am taking here is just annotating it like this so it gets ignored:
This is good because it forces one to be explicit about it (you add it when you know the docstrings should be inherited) and is less complex than implementing something on our own. Although, @pauladkisson already implemented an action that I think accomplishes this if you prefer to avoid having to add #noqa annotations
codecov[bot]
commented
5 days ago All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 90.58%. Comparing base (
36464df) to head (716d400). Report is 16 commits behind head on main.
bendichter
commented
5 days ago I tend to side more on not adding chores for devs. They tend to pile up in a way that creates a lot of busy work and makes it difficult to onboard people and is unwelcoming to new contributors. In this particular case I suppose I could go either way
Should come after #1062
This was the most annoying one.