Conversation
Summary of ChangesHello @asaiacai, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly elevates the project's code quality and maintainability by integrating Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on refactoring the codebase for improved type hinting, consistency in string literal usage (changing double quotes to single quotes), and minor code cleanup across various modules. Key changes include updating README badges and URLs to reflect new branding, adding comprehensive type annotations to variables and function signatures in __init__.py, file.py, iface.py, init.py, op.py, sets.py, store.py, and sys.py. Error messages and warnings have been refined for better readability, and some critical logging statements were replaced with ValueError exceptions for more robust error handling, particularly in video data processing. Additionally, the pyproject.toml file was updated to include ignore_missing_imports = true for MyPy configuration. Review comments highlighted the need to address mutable default arguments in make_compat_graph_nodes_v1, get_shape, and Graph.__init__ to prevent unexpected behavior and state leakage, and to ensure self._dataframe is properly handled in Table.to_data to avoid AttributeError.
| name = "." | ||
| elif p == ".": | ||
| name = str(d["name"]) | ||
| def make_compat_graph_nodes_v1(d, ref, dep=0, p='', r={}): |
There was a problem hiding this comment.
Using a mutable default argument like r={} can lead to unexpected behavior, as the dictionary will be shared across all calls to this function that don't explicitly provide r. This can cause state to leak between different graph constructions. It's safer to initialize r to None and create a new dictionary inside the function if r is None.
After changing the signature, please add if r is None: r = {} as the first line of the function.
| def make_compat_graph_nodes_v1(d, ref, dep=0, p='', r={}): | |
| def make_compat_graph_nodes_v1(d, ref, dep=0, p='', r=None): |
| return info | ||
|
|
||
|
|
||
| def get_shape(tensor, r=set()): |
There was a problem hiding this comment.
Using a mutable default argument r=set() is dangerous because the set will be shared across different calls to get_shape. This means that visited nodes from one call will persist and affect subsequent calls, potentially leading to incorrect shape calculations if the function is used on different tensors. To avoid this, you should default to None and create a new set within the function.
After changing the signature, please add if r is None: r = set() as the first line of the function.
| def get_shape(tensor, r=set()): | |
| def get_shape(tensor, r=None): |
| self._pd = self._dataframe.to_dict(orient="split") | ||
| self._table = self._pd.get("data", [[None]]) | ||
| if data.__class__.__name__ == 'DataFrame' and hasattr(data, 'to_dict'): | ||
| self._pd = self._dataframe.to_dict(orient='split') |
There was a problem hiding this comment.
There's a potential AttributeError here. The code uses self._dataframe.to_dict(), but self._dataframe could be None if a DataFrame is passed via the data positional argument instead of the dataframe keyword argument. You should use the data variable which holds the correct object in both cases.
| self._pd = self._dataframe.to_dict(orient='split') | |
| self._pd = data.to_dict(orient='split') |
| tag = "Graph" | ||
| tag = 'Graph' | ||
|
|
||
| def __init__(self, data={}): |
There was a problem hiding this comment.
Using a mutable default argument like data={} is generally discouraged. While it might seem to work here because you're only reading from it with .get(), it can lead to subtle bugs if the code is changed later. It's safer to use data=None and initialize it to an empty dictionary inside the function.
After changing the signature, please add if data is None: data = {} as the first line of the function.
| def __init__(self, data={}): | |
| def __init__(self, data=None): |
adds linting and types