-
Notifications
You must be signed in to change notification settings - Fork 1
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
Distributing typing information (#127) #135
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe changes introduced in this pull request encompass the addition of new configuration settings in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Config
participant Aggregator
participant Constraints
participant Utils
User->>Config: Load configuration
Config->>Aggregator: Provide aggregation settings
User->>Aggregator: Call aggregate functions
Aggregator->>Utils: Log aggregation process
User->>Constraints: Check constraints on data
Constraints->>Utils: Log constraint checks
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Should typing information be included inline and also with these stubs @mmcdermott? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
|
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.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (7)
src/aces/query.pyi (1)
8-8
: LGTM! Consider adding a docstring for better documentation.The function signature is well-defined with clear type hints. However, to improve the documentation, consider adding a docstring to describe the purpose of the function, its parameters, and the return value.
Here's a suggested docstring to add above the function signature:
def query(cfg: TaskExtractorConfig, predicates_df: pl.DataFrame) -> pl.DataFrame: """ Query and process data based on the given configuration and predicates. Args: cfg (TaskExtractorConfig): Configuration for task extraction. predicates_df (pl.DataFrame): DataFrame containing predicates for querying. Returns: pl.DataFrame: Resulting DataFrame after applying the query and processing. """ ...src/aces/extract_subtree.pyi (1)
6-11
: LGTM: Well-structured function signature with clear type annotations.The function signature is well-defined with appropriate type annotations, which enhances maintainability and IDE support. The parameter names are descriptive, improving readability.
Consider adding a docstring to provide more context about the function's purpose and parameter descriptions. This would further improve the usability of the stub file. For example:
def extract_subtree( subtree: Node, subtree_anchor_realizations: pl.DataFrame, predicates_df: pl.DataFrame, subtree_root_offset: timedelta = timedelta(0), ) -> pl.DataFrame: """ Extract a subtree based on the provided parameters. Args: subtree (Node): The subtree to extract. subtree_anchor_realizations (pl.DataFrame): DataFrame containing anchor realizations. predicates_df (pl.DataFrame): DataFrame containing predicates. subtree_root_offset (timedelta, optional): Offset for the subtree root. Defaults to timedelta(0). Returns: pl.DataFrame: The extracted subtree data. """ ...src/aces/utils.pyi (1)
16-16
: Consider a more descriptive name for log_tree function.While the
log_tree
function signature is clear, the function name could be more descriptive. Consider renaming it to something likelog_node_tree
orlog_tree_structure
to better convey its purpose.src/aces/utils.py (1)
Line range hint
1-70
: Reconsider the approach to distributing typing information.The removal of type hints from these functions seems to be part of the PR objective "Distributing typing information". However, this approach may not be ideal for several reasons:
- It reduces code clarity and self-documentation in the main implementation file.
- It may lead to inconsistencies between the implementation and any separate type definitions.
- It goes against the trend in the Python community towards more type annotations in code.
Instead of removing type hints from the main implementation, consider:
- Keeping type hints in the main implementation file for clarity and immediate understanding.
- If the goal is to provide separate type definitions, consider using stub files (
.pyi
) in addition to, not instead of, in-line type hints. This allows for more detailed or alternative type definitions without sacrificing the benefits of in-line hints.- Ensure that any separate type definitions (if used) are consistent with the in-line hints and are kept up-to-date with changes in the implementation.
This approach would maintain code quality in the main implementation while still allowing for the distribution of additional typing information if needed.
src/aces/predicates.pyi (1)
1-7
: Organize import statements for improved readability.
- Group imports in the order: standard library imports, third-party imports, local imports.
- Include necessary imports from the
typing
module due to added type hints.- Remove any unused imports to keep the code clean.
Apply this diff to reorganize and update imports:
from pathlib import Path +from typing import Any, Dict, List, Optional import polars as pl from omegaconf import DictConfig from .config import TaskExtractorConfig
src/aces/config.pyi (2)
31-31
: Confirm the necessity of the empty__post_init__
method inDerivedPredicateConfig
.The
__post_init__
method is declared but contains...
, indicating no current implementation. If it's not performing any initialization, it might be unnecessary.Consider removing the
__post_init__
method if it's not needed, or provide an implementation if future initialization steps are anticipated.
90-95
: Simplify type annotations in theload
method ofTaskExtractorConfig
.In the
load
method, the class typeTaskExtractorConfig
is quoted in the type annotations. Since we're within the class definition, and forward references are not needed here, the quotes can be omitted.Update the type annotations for clarity:
- cls: Type["TaskExtractorConfig"], + cls: Type[TaskExtractorConfig], ... - ) -> "TaskExtractorConfig": ... + ) -> TaskExtractorConfig: ...This makes the annotations more straightforward and avoids unnecessary use of forward references.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- pyproject.toml (1 hunks)
- src/aces/aggregate.pyi (1 hunks)
- src/aces/config.pyi (1 hunks)
- src/aces/constraints.pyi (1 hunks)
- src/aces/extract_subtree.pyi (1 hunks)
- src/aces/predicates.pyi (1 hunks)
- src/aces/query.pyi (1 hunks)
- src/aces/types.pyi (1 hunks)
- src/aces/utils.py (2 hunks)
- src/aces/utils.pyi (1 hunks)
🔇 Additional comments not posted (26)
src/aces/query.pyi (2)
1-8
: Overall, the type stub file is well-structured and provides clear type hints.The file successfully defines the type information for the
query
function, which appears to be a key component in a task extraction process. The use of Polars DataFrames suggests efficient data handling. To further improve the file:
- Verify the usage of imported modules as suggested earlier.
- Consider adding a docstring to the
query
function for better documentation.These minor improvements will enhance the overall quality and maintainability of the code.
1-6
: Verify the usage of imported modulesThe following imports are not directly used in this type stub file:
Optional
fromtyping
Node
frombigtree
While they might be used in the implementation file, it's good practice to keep imports minimal in stub files.
Please run the following script to check if these imports are used in the corresponding implementation file:
If these imports are not used in the implementation file, consider removing them from the stub file.
✅ Verification successful
Unused imports confirmed
The imports
Optional
fromtyping
andNode
frombigtree
are not used in either the stub filesrc/aces/query.pyi
or the implementation filesrc/aces/query.py
. It is recommended to remove these imports from the stub file to keep the codebase clean and maintainable.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if unused imports in the stub file are used in the implementation file # Test: Search for usage of Optional and Node in the implementation file implementation_file=$(echo "src/aces/query.pyi" | sed 's/\.pyi$/.py/') echo "Checking usage in $implementation_file" rg --type python "Optional|Node" "$implementation_file"Length of output: 254
Script:
#!/bin/bash # Description: Verify if 'Optional' and 'Node' are used in the implementation file implementation_file="src/aces/query.py" if [ -f "$implementation_file" ]; then echo "Checking usage of 'Optional' and 'Node' in $implementation_file" rg "Optional|Node" "$implementation_file" else echo "Implementation file $implementation_file does not exist." echo "Please ensure that the implementation file is present." fiLength of output: 252
src/aces/extract_subtree.pyi (2)
1-4
: LGTM: Imports are appropriate and well-structured.The imports are relevant to the function being defined:
timedelta
is used in the function signature.polars
(aspl
) is used for DataFrame operations.Node
frombigtree
is used for tree-like data structures.These choices suggest a focus on performance and appropriate data structures for the task at hand.
1-11
: Overall, excellent stub file implementation.This stub file is well-structured and provides clear type information for the
extract_subtree
function. The use of type annotations, appropriate imports, and a well-defined function signature contributes to improved maintainability and developer experience. The only suggested improvement is the addition of a docstring for enhanced documentation.src/aces/constraints.pyi (5)
6-8
: LGTM! Well-defined function signature.The
check_constraints
function signature is well-typed and clearly defines its input parameters and return type. The use of type hints is appropriate and will aid in type checking and code understanding.
9-9
: LGTM! Well-defined function signature.The
check_static_variables
function signature is well-typed and clearly defines its input parameters and return type. The use of type hints is appropriate and will aid in type checking and code understanding.
1-9
: Overall, the constraints.pyi file looks good.This file provides clear type hints for the
check_constraints
andcheck_static_variables
functions, which will improve type checking and code understanding. The function signatures are well-defined and use appropriate type hints.A few suggestions for improvement:
- Verify the usage of the
logger
import from theloguru
library.- Confirm the usage of the
ANY_EVENT_COLUMN
import from the local.types
module.If these imports are not used in the actual implementation, consider removing them to keep the type hints file concise and focused.
4-4
: LGTM! Verify ANY_EVENT_COLUMN usage.The import from a local module is good for maintaining a modular structure.
However,
ANY_EVENT_COLUMN
is not visibly used in this file. Let's verify its usage:#!/bin/bash # Description: Verify the usage of ANY_EVENT_COLUMN in the constraints module. # Test: Search for ANY_EVENT_COLUMN usage in the constraints module rg --type python 'ANY_EVENT_COLUMN' src/aces/constraints.pyIf the script doesn't return any results, consider removing the unused import.
1-2
: LGTM! Verify logger usage.The imports look good. The Polars library is correctly imported and used in the function signatures.
However, the
logger
import is not visibly used in this file. Let's verify its usage:If the script doesn't return any results, consider removing the unused import.
src/aces/utils.pyi (5)
1-11
: LGTM: Import statements are well-organized and appropriate.The import statements are properly structured, separating standard library imports from third-party imports. They cover all the necessary modules for the utility functions declared in this file.
13-13
: LGTM: parse_timedelta function signature is well-defined.The function signature for
parse_timedelta
is clear and uses appropriate type hints. It correctly handles an optional input and returns atimedelta
object.
14-15
: LGTM: capture_output context manager is well-defined.The
capture_output
context manager is correctly decorated and has an appropriate return type hint for a generator that yields aStringIO
object.
17-17
: LGTM: hydra_loguru_init function signature is well-defined.The
hydra_loguru_init
function signature is clear and uses appropriate type hints. It correctly specifies a filename input and indicates that it doesn't return a value.
1-17
: Overall, the utils.pyi stub file is well-structured and provides clear type information.This stub file effectively defines the type information for the utility functions in the
aces.utils
module. The use of type hints and the structure of the file are appropriate for a.pyi
stub file. The only suggestion is to consider renaming thelog_tree
function for better clarity, but this is a minor improvement.pyproject.toml (3)
32-33
: LGTM: Enabling package data inclusionThe addition of
include-package-data = true
in the[tool.setuptools]
section is correct and aligns with the PR objective. This configuration ensures that package data, including typing information files, will be included when building the package.
32-36
: Summary: Proper configuration for distributing typing informationThe changes in this file correctly set up the project for distributing typing information:
- Enabling
include-package-data
ensures all package data is included during build.- Specifying
*.pyi
andpy.typed
files as package data ensures type information is distributed.These changes align well with the PR objective and follow best practices for Python packaging. Once the package name is verified and corrected if necessary, these modifications will effectively enable the distribution of typing information with your package.
35-36
: Verify the package name and approve type information inclusionThe inclusion of
*.pyi
andpy.typed
files as package data is correct and aligns with the PR objective of distributing typing information. This ensures that type hint stub files and the marker file for type checking support are included in the package.However, please verify if
your_package
is the correct package name. It appears to be a placeholder and should be replaced with the actual package name (likely "es-aces" based on the project name).To confirm the correct package name, you can run the following command:
If the result doesn't match "your_package", please update the package name accordingly.
✅ Verification successful
Package name verified and type information inclusion approved
- Replace
your_package
withes-aces
in the[tool.setuptools.package-data]
section.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct package name in pyproject.toml # Test: Search for the package name in pyproject.toml rg --type toml 'name\s*=\s*"([^"]+)"' pyproject.tomlLength of output: 194
src/aces/aggregate.pyi (1)
7-10
: 🛠️ Refactor suggestionEnsure type annotations are compatible with supported Python versions
The use of the
|
operator for type unions (e.g.,TemporalWindowBounds | tuple[...]
andtimedelta | None
) requires Python 3.10 or later. If the project needs to support earlier Python versions, consider usingUnion
andOptional
from thetyping
module to maintain compatibility.You can verify the project's Python version compatibility by checking the
pyproject.toml
orsetup.py
. If earlier versions need to be supported, apply this diff:+from typing import Union, Optional -def aggregate_temporal_window( - predicates_df: pl.DataFrame, - endpoint_expr: TemporalWindowBounds | tuple[bool, timedelta, bool, timedelta | None], -) -> pl.DataFrame: ... +def aggregate_temporal_window( + predicates_df: pl.DataFrame, + endpoint_expr: Union[TemporalWindowBounds, tuple[bool, timedelta, bool, Optional[timedelta]]], +) -> pl.DataFrame: ...Run the following script to check the Python version specified in
pyproject.toml
:src/aces/predicates.pyi (1)
20-20
: Confirm the accessibility ofTaskExtractorConfig
.The function
get_predicates_df
usesTaskExtractorConfig
as a parameter type. Verify thatTaskExtractorConfig
is properly defined and accessible from.config
. This ensures that type checking and code navigation work correctly.src/aces/types.pyi (4)
6-11
: Global variables are declared appropriately.The global variables
PRED_CNT_TYPE
,START_OF_RECORD_KEY
,END_OF_RECORD_KEY
, andANY_EVENT_COLUMN
are correctly declared with appropriate type annotations.
12-24
:TemporalWindowBounds
data class is well-defined.The
TemporalWindowBounds
class and its methods are correctly declared with appropriate type annotations and method signatures.
25-36
:ToEventWindowBounds
data class is well-defined.The
ToEventWindowBounds
class and its methods are correctly declared with appropriate type annotations and method signatures.
1-5
:⚠️ Potential issueAdd missing import for
dataclasses
.The
dataclasses
module is used but not imported. Please add the import statement fordataclasses
to ensure the decorators function correctly.Apply this diff to fix the issue:
from datetime import timedelta +import dataclasses from typing import Any, Iterator import polars as pl
Likely invalid or redundant comment.
src/aces/config.pyi (3)
51-61
: Check return types of property methods inWindowConfig
.The property methods
start_endpoint_expr
andend_endpoint_expr
returnOptional[Union[ToEventWindowBounds, TemporalWindowBounds]]
. Ensure that these types are correctly defined and imported.Confirm that
ToEventWindowBounds
andTemporalWindowBounds
are properly defined:#!/bin/bash # Description: Verify the definitions of ToEventWindowBounds and TemporalWindowBounds. # Search for the type definitions in the codebase. rg --type py --files-with-matches 'class ToEventWindowBounds' rg --type py --files-with-matches 'class TemporalWindowBounds'
45-48
: Verify the implementations of_check_reference
and_parse_boundary
methods inWindowConfig
.The class methods
_check_reference
and_parse_boundary
are declared with...
, suggesting their implementations should be provided elsewhere.Ensure these methods are implemented in the codebase:
#!/bin/bash # Description: Verify the existence of method implementations for _check_reference and _parse_boundary. # Search for the implementations in `.py` files excluding stub files. rg --type py --glob '!*.pyi' -l 'def _check_reference' rg --type py --glob '!*.pyi' -l 'def _parse_boundary'
20-21
: Ensure implementations ofMEDS_eval_expr
andESGPT_eval_expr
exist.The methods
MEDS_eval_expr
andESGPT_eval_expr
inPlainPredicateConfig
are declared with...
, indicating that their implementations should exist elsewhere.Run the following script to confirm that these methods are implemented in the codebase:
✅ Verification successful
Method Implementations Confirmed
The implementations of
MEDS_eval_expr
andESGPT_eval_expr
exist insrc/aces/config.py
.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of method implementations for MEDS_eval_expr and ESGPT_eval_expr. # Search for the implementations in `.py` files excluding stub files. rg --type py --glob '!*.pyi' -l 'def MEDS_eval_expr' rg --type py --glob '!*.pyi' -l 'def ESGPT_eval_expr'Length of output: 258
Script:
#!/bin/bash # Description: Verify the existence of method implementations for MEDS_eval_expr and ESGPT_eval_expr. # Search for the implementation of MEDS_eval_expr in `.py` files excluding stub files. rg --type py --glob '!*.pyi' -l 'def MEDS_eval_expr' # Search for the implementation of ESGPT_eval_expr in `.py` files excluding stub files. rg --type py --glob '!*.pyi' -l 'def ESGPT_eval_expr'Length of output: 148
@@ -67,7 +67,7 @@ def log_tree(node): | |||
logger.info("\n" + captured.getvalue()) # Log the captured output | |||
|
|||
|
|||
def hydra_loguru_init(filename) -> None: | |||
def hydra_loguru_init(filename): |
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.
Consider retaining return type hint and adding parameter type hint.
The removal of the return type hint from the hydra_loguru_init
function may reduce code clarity and self-documentation. Additionally, the filename
parameter lacks a type hint.
Consider reverting the change and adding a type hint for the filename
parameter:
-def hydra_loguru_init(filename):
+def hydra_loguru_init(filename: str) -> None:
This change:
- Maintains code quality and clarity.
- Explicitly indicates that the function doesn't return a value.
- Adds type information for the
filename
parameter, improving code documentation. - Enables better tooling support (IDE suggestions, linter warnings, etc.).
- Aligns with Python's trend towards more type annotations (PEP 484).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
def hydra_loguru_init(filename): | |
def hydra_loguru_init(filename: str) -> None: |
@@ -10,7 +10,7 @@ | |||
from pytimeparse import parse | |||
|
|||
|
|||
def parse_timedelta(time_str: str) -> timedelta: | |||
def parse_timedelta(time_str): |
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.
Consider retaining type hints for improved code quality and consistency.
The removal of type hints from the parse_timedelta
function signature may have unintended consequences:
- It reduces code clarity and self-documentation.
- It may lead to runtime errors if incorrect types are passed.
- It loses the benefit of static type checking.
- It's inconsistent with Python's trend towards more type annotations (PEP 484).
- The docstring still mentions parameter and return types, which is now inconsistent with the signature.
Consider reverting the change and keeping the type hints:
-def parse_timedelta(time_str):
+def parse_timedelta(time_str: str) -> timedelta:
This maintains code quality, consistency with the docstring, and enables better tooling support (IDE suggestions, linter warnings, etc.).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
def parse_timedelta(time_str): | |
def parse_timedelta(time_str: str) -> timedelta: |
class PlainPredicateConfig: | ||
pass | ||
|
||
class DerivedPredicateConfig: | ||
input_predicates: Any | ||
|
||
class WindowConfig: | ||
referenced_predicates: set[str] | ||
label: Optional[str] | ||
index_timestamp: Optional[str] | ||
start_endpoint_expr: Optional[str] | ||
end_endpoint_expr: Optional[str] | ||
has: dict[str, Any] | ||
root_node: str | ||
referenced_event: list[str] | ||
|
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.
Avoid duplicating class definitions for PlainPredicateConfig
, DerivedPredicateConfig
, and WindowConfig
.
Classes PlainPredicateConfig
, DerivedPredicateConfig
, and WindowConfig
are defined again starting at line 67. This duplication can cause confusion and potential type checking issues.
Consider removing the duplicate class definitions to prevent conflicts:
# Assuming these classes are defined elsewhere
class EventConfig:
predicate: str
-class PlainPredicateConfig:
- pass
-
-class DerivedPredicateConfig:
- input_predicates: Any
-
-class WindowConfig:
- referenced_predicates: set[str]
- label: Optional[str]
- index_timestamp: Optional[str]
- start_endpoint_expr: Optional[str]
- end_endpoint_expr: Optional[str]
- has: dict[str, Any]
- root_node: str
- referenced_event: list[str]
If these classes are meant to be placeholders or interfaces, consider renaming them or ensuring they are only defined once.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
class PlainPredicateConfig: | |
pass | |
class DerivedPredicateConfig: | |
input_predicates: Any | |
class WindowConfig: | |
referenced_predicates: set[str] | |
label: Optional[str] | |
index_timestamp: Optional[str] | |
start_endpoint_expr: Optional[str] | |
end_endpoint_expr: Optional[str] | |
has: dict[str, Any] | |
root_node: str | |
referenced_event: list[str] | |
class EventConfig: | |
predicate: str |
class TaskExtractorConfig: | ||
predicates: Dict[str, Union[PlainPredicateConfig, DerivedPredicateConfig]] | ||
trigger: EventConfig | ||
windows: Optional[Dict[str, WindowConfig]] | ||
label_window: Optional[str] | ||
index_timestamp_window: Optional[str] |
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.
Provide default values for optional fields in TaskExtractorConfig
.
The fields windows
, label_window
, and index_timestamp_window
are annotated as optional but don't have default values. This might lead to TypeError
if they're not supplied during instantiation.
Assign default values to optional fields:
- windows: Optional[Dict[str, WindowConfig]]
+ windows: Optional[Dict[str, WindowConfig]] = None
- label_window: Optional[str]
+ label_window: Optional[str] = None
- index_timestamp_window: Optional[str]
+ index_timestamp_window: Optional[str] = None
This ensures that instances of TaskExtractorConfig
can be created without explicitly providing these optional parameters.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
class TaskExtractorConfig: | |
predicates: Dict[str, Union[PlainPredicateConfig, DerivedPredicateConfig]] | |
trigger: EventConfig | |
windows: Optional[Dict[str, WindowConfig]] | |
label_window: Optional[str] | |
index_timestamp_window: Optional[str] | |
class TaskExtractorConfig: | |
predicates: Dict[str, Union[PlainPredicateConfig, DerivedPredicateConfig]] | |
trigger: EventConfig | |
windows: Optional[Dict[str, WindowConfig]] = None | |
label_window: Optional[str] = None | |
index_timestamp_window: Optional[str] = None |
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.
@justin13601, this comment should also likely apply to other optional parameters.
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.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
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.
@justin13601 this looks good to me, though i don't know how the .pyi
files usually work / should be formatted. Note that any remaining, non-resolved coderabbit comments are things that I agree with and recommend should be addressed before you push the PR.
class TaskExtractorConfig: | ||
predicates: Dict[str, Union[PlainPredicateConfig, DerivedPredicateConfig]] | ||
trigger: EventConfig | ||
windows: Optional[Dict[str, WindowConfig]] | ||
label_window: Optional[str] | ||
index_timestamp_window: Optional[str] |
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.
@justin13601, this comment should also likely apply to other optional parameters.
Yes, I think so, but google around or check the google style guide to see if there are other recommendations in the community. |
@justin13601 can we merge or close this? Not sure which would be more appropriate, but I'd like to get it closed out rather than remaining indefinitely open. Thanks! |
Yep will try to merge this weekend! |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation