-
-
Notifications
You must be signed in to change notification settings - Fork 157
TYP: Clean up annotations for undocumented elements #1563
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
Conversation
cmp0xff
left a comment
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.
I am not familar with the things under computation in this PR. May I invite @Dr-Irv to review?
| class AbstractEngine(metaclass=abc.ABCMeta): | ||
| has_neg_frac: bool = ... | ||
| expr = ... | ||
| aligned_axes = ... | ||
| result_type = ... | ||
| def __init__(self, expr) -> None: ... | ||
| def convert(self) -> str: ... | ||
| def evaluate(self) -> object: ... | ||
|
|
||
| class NumExprEngine(AbstractEngine): | ||
| has_neg_frac: bool = ... | ||
|
|
||
| class PythonEngine(AbstractEngine): | ||
| has_neg_frac: bool = ... |
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.
These are not documented. Can they be removed?
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.
I tried to do the minimum to make the ANN pass, we could do a second round of cutting to drop some of the elements that are unused. Open to it
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.
delete this file. Nothing in here is documented.
Dr-Irv
left a comment
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.
I think most of the files in core/computation can be deleted EXCEPT class Expr in expr.pyi
That ends up being needed by read_hdf so give that a try.
| class AbstractEngine(metaclass=abc.ABCMeta): | ||
| has_neg_frac: bool = ... | ||
| expr = ... | ||
| aligned_axes = ... | ||
| result_type = ... | ||
| def __init__(self, expr) -> None: ... | ||
| def convert(self) -> str: ... | ||
| def evaluate(self) -> object: ... | ||
|
|
||
| class NumExprEngine(AbstractEngine): | ||
| has_neg_frac: bool = ... | ||
|
|
||
| class PythonEngine(AbstractEngine): | ||
| has_neg_frac: bool = ... |
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.
delete this file. Nothing in here is documented.
| from pandas.core.computation.ops import Term as Term | ||
| from pandas.core.computation.scope import Scope as Scope | ||
|
|
||
| class BaseExprVisitor(ast.NodeVisitor): |
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.
delete this class. Only referenced in pytables to create other classes that are not documented
| def visit_Compare(self, node, **kwargs: Any): ... | ||
| def visit_BoolOp(self, node, **kwargs: Any): ... | ||
|
|
||
| class Expr: |
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.
need to keep this one because it is superclass of PyTablesExpr which is same as Term which is arguments for read_hdf()
Now we don't document what Term is, but safer to leave it in for now
| def __init__(self, name, is_local: bool = ...) -> None: ... | ||
| class UndefinedVariableError(NameError): ... | ||
|
|
||
| class Term: |
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.
I tihnk Term can be deleted
| class Constant(Term): | ||
| @property | ||
| def name(self): ... | ||
| class Constant(Term): ... |
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.
can be deleted
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.
I agree, can be deleted
| def name(self): ... | ||
| class Constant(Term): ... | ||
|
|
||
| class Op: |
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.
I think that this and its subclasses can be deleted
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.
I think this can be deleted.
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.
PyTables is used in pandas/io/pytables.pyi so could not delete the file in full but dropped most of it.
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.
I think this file can be deleted.
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.
can delete this file as well
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.
Actually Scope is used in expr file (everything is so spread around this folder).
cmp0xff
left a comment
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.
I find more places that can be deleted. But if the purpose is just to remove untyped methods, I can approve.
I don't see anything removed that shouldn't be.
Thank you @loicdiridollou !
| def prune(self, klass): ... | ||
| class UnaryOp(ops.UnaryOp): ... | ||
|
|
||
| class PyTablesExprVisitor(BaseExprVisitor): |
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.
This one isn't referred to anywhere else.
|
|
||
| class UnaryOp(ops.UnaryOp): | ||
| def prune(self, klass): ... | ||
| class UnaryOp(ops.UnaryOp): ... |
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.
This one isn't referred to anywhere else.
| def format(self): ... | ||
| condition = ... | ||
|
|
||
| class JointConditionBinOp(ConditionBinOp): |
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.
This one isn't referred to anywhere else.
| class JointFilterBinOp(FilterBinOp): | ||
| def format(self) -> None: ... | ||
|
|
||
| class ConditionBinOp(BinOp): |
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.
This one isn't referred to anywhere else.
| def format(self): ... | ||
| def generate_filter_op(self, invert: bool = ...): ... | ||
|
|
||
| class JointFilterBinOp(FilterBinOp): |
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.
This one isn't referred to anywhere else.
| class Constant(Term): | ||
| def __init__(self, name, env: PyTablesScope, side=..., encoding=...) -> None: ... | ||
|
|
||
| class BinOp(ops.BinOp): |
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.
This one is referred to only in classes that are not referred to. Can also be removed, I believe.
| class Constant(Term): | ||
| @property | ||
| def name(self): ... | ||
| class Constant(Term): ... |
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.
I agree, can be deleted
|
Please merge main so we can proceed. |
cmp0xff
left a comment
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.
I've double checked after merging main.
assert_type()to assert the type of any return value)AGENTS.md.