Chore: Add docs for callback, add type alias #447
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Possibly related PRs
Wanna explore any of these changes further or discuss potential implications? wdyt? 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
airbyte/callbacks.py (2)
10-10: Consider using TypedDict for better type safety?The current type alias works well, but we could make it even more type-safe. What do you think about defining a
ConfigDictTypedDict to specify the expected structure of the configuration dictionary? This would provide better type hints for callback implementations. wdyt?from typing import TypedDict class ConfigDict(TypedDict): # Add expected config keys here pass ConfigChangeCallback = Callable[[ConfigDict], None]
11-50: Excellent documentation! Maybe add error handling to the example?The documentation is comprehensive and well-structured! 🌟 The example is particularly helpful. Would you consider adding error handling to make it even more production-ready? Something like:
def config_callback(new_config: dict[str, Any]) -> None: try: config_file.write_text(yaml.safe_dump(new_config)) except (IOError, yaml.YAMLError) as e: logger.error(f"Failed to save updated config: {e}") # Consider whether to re-raise or handle differentlyAlso, should we mention that the callback might be called from a different thread? wdyt?
airbyte/destinations/util.py (1)
25-25: The type update looks great! Quick question about the docs though...The parameter type update to
ConfigChangeCallbackimproves type safety nicely. Would you consider enhancing the docstring forconfig_change_callbackto describe its expected signature? Something like "callback function (ConfigChangeCallback) that takes a config dict and returns None" perhaps? wdyt? 🤔airbyte/sources/util.py (1)
51-51: The type annotation looks great! Maybe we could enhance the docstring a bit?The ConfigChangeCallback parameter is well-typed. Would you like to add a bit more detail to the docstring about what kind of changes trigger the callback and maybe add an example? wdyt?
airbyte/sources/base.py (2)
63-64: Consider enhancing parameter documentation?The new parameters could benefit from docstring updates. What do you think about adding:
config_change_callback: Description of when and how the callback is invokedstreams: Description of the expected format and behavior when using string vs list[str]
Line range hint
447-452: Consider enhancing error handling for streams argument?The error message could be more helpful by including the actual expected types. What do you think about:
- raise exc.PyAirbyteInputError( - message="Invalid streams argument.", - input_value=streams, - ) + raise exc.PyAirbyteInputError( + message="Invalid streams argument. Expected '*' or list[str].", + input_value=streams, + expected_types=["'*'", "list[str]"], + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
airbyte/__init__.py(2 hunks)airbyte/_connector_base.py(2 hunks)airbyte/callbacks.py(1 hunks)airbyte/destinations/base.py(2 hunks)airbyte/destinations/util.py(1 hunks)airbyte/sources/base.py(2 hunks)airbyte/sources/util.py(2 hunks)
🔇 Additional comments (13)
airbyte/callbacks.py (2)
1-7: Clean and well-structured module setup!
The imports are minimal and specific, and the module structure follows best practices. Nice work! 🎉
10-50: Verify thread safety requirements
Let's check if the callback might be called from different threads to ensure we provide appropriate guidance in the documentation.
airbyte/destinations/util.py (2)
18-18: Nice type safety improvement! 👍
The addition of ConfigChangeCallback import and its placement within the TYPE_CHECKING block looks great. This aligns well with the type safety improvements across the codebase.
25-25: Quick verification of consistent usage?
Let's make sure this type update is consistent across the codebase:
✅ Verification successful
Type usage appears consistent across the codebase
The ConfigChangeCallback type is used consistently in all relevant locations:
_connector_base.pysources/base.pysources/util.pydestinations/base.pydestinations/util.py
No instances of the old Callable[[dict[str, Any]], None] type were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining Callable[[dict[str, Any]], None] type annotations
# that should be using ConfigChangeCallback instead
# Search for potential callback parameter definitions
rg "config_change_callback.*:.*Callable\[\[dict\[str, Any\]\], None\]"
# Search for ConfigChangeCallback usage to confirm consistent adoption
rg "config_change_callback.*:.*ConfigChangeCallback"
Length of output: 592
airbyte/sources/util.py (2)
18-18: LGTM! Clean type import addition.
Nice job putting the ConfigChangeCallback import within the TYPE_CHECKING block! This keeps the runtime imports clean. 👍
Line range hint 21-40: The deprecation path looks good! Shall we verify the migration docs?
The deprecation warning is clear and helpful. Would you like me to check if this deprecation is documented in the changelog or migration guide?
airbyte/__init__.py (1)
128-128: LGTM! Clean addition of the callbacks module.
The changes look good - the new callbacks module is properly integrated into both the imports and __all__ list, maintaining alphabetical order and following the existing code style. This aligns well with the PR's objective of improving type safety through the new callback functionality.
Let's verify the callbacks module structure:
Also applies to: 161-161
✅ Verification successful
Looks great! The callbacks module is properly integrated and well-used across the codebase
The verification shows that:
- The
callbacks.pymodule exists and definesConfigChangeCallbacktype - It's properly imported and used in key components:
- Base connector classes
- Source and destination utilities
- Integration tests validate the callback functionality
- The implementation follows a clean type-safe pattern with proper documentation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the callbacks module structure and its public interface
# Test: Check if the callbacks module exists and contains expected type definitions
rg -A 5 "ConfigChangeCallback" --type python
Length of output: 75
Script:
#!/bin/bash
# Let's try a different approach to verify the callbacks module
# Check if callbacks module exists and its content
fd "callbacks.py" --type f
# Look for any files containing "callback" in their name
fd "callback" --type f
# Search for callback-related code using ripgrep
rg -g "!node_modules" -g "!.git" "ConfigChangeCallback"
# Check the content of any Python files in the airbyte directory
fd ".py$" --type f --exec cat {} \; | rg -A 5 "callback"
Length of output: 8729
airbyte/destinations/base.py (2)
38-38: LGTM! Nice type safety improvement.
The addition of ConfigChangeCallback import in the TYPE_CHECKING block is clean and follows best practices. This aligns well with the PR's goal of improving type hints.
53-53: Type alias change looks good! Quick verification needed.
The change from Callable[[dict[str, Any]], None] | None to ConfigChangeCallback | None improves readability. Would you mind if we verify that all callers are compatible with this change? 🤔
✅ Verification successful
Let me check the type definition of ConfigChangeCallback to ensure complete verification.
All good! The type change is perfectly compatible 👍
I found that ConfigChangeCallback is defined in airbyte/callbacks.py as Callable[[dict[str, Any]], None], which is exactly the same type as before. The change is purely for better readability and all imports/usages are consistent across the codebase. Nice refactoring!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct instantiations of Destination class to verify compatibility
rg -A 2 "Destination\("
# Search for any references to config_change_callback to ensure type consistency
rg "config_change_callback.*?:"
Length of output: 1721
Script:
#!/bin/bash
# Search for ConfigChangeCallback type definition
rg "ConfigChangeCallback.*?=" -A 2
# Also check for any imports of this type
rg "from.*?import.*?ConfigChangeCallback"
Length of output: 1743
airbyte/_connector_base.py (2)
39-39: LGTM! Clean import changes.
The removal of unused Callable import and addition of ConfigChangeCallback improves code organization.
Also applies to: 44-44
61-61: LGTM! Type annotation improvement.
The switch to ConfigChangeCallback type alias enhances code readability and type safety. The implementation correctly preserves the callback functionality in both the constructor and message processing logic.
airbyte/sources/base.py (2)
46-46: LGTM! Clean import addition.
The addition of ConfigChangeCallback import aligns well with the type safety improvements mentioned in the PR objectives.
Line range hint 89-97: LGTM! Well-implemented deprecation.
The deprecation warning is clear and helpful, providing users with the correct migration path to select_streams.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
airbyte/callbacks.py (2)
10-10: Consider adding runtime type checking?The type alias looks great! For enhanced developer experience, what do you think about adding runtime type checking using
typing.runtime_checkableandProtocol? This would provide better error messages during development. wdyt?from typing import Protocol, runtime_checkable @runtime_checkable class ConfigChangeCallback(Protocol): def __call__(self, config: dict[str, Any]) -> None: ...
11-50: Great docs! A couple of suggestions to make them even betterThe documentation is comprehensive and well-structured! 👏 Would you consider these enhancements?
- The example could be more realistic by showing an OAuth refresh token scenario since it's mentioned as the most common use case. Maybe something like:
def config_callback(new_config: dict[str, Any]) -> None: # Store the new refresh token securely keyring.set_password( "my-oauth-service", "refresh_token", new_config["credentials"]["refresh_token"] )
- Consider adding a security note about handling sensitive config data, especially for OAuth tokens? Something like:
"Note: When implementing the callback, ensure sensitive configuration data is stored securely."wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
airbyte/callbacks.py(1 hunks)
🔇 Additional comments (1)
airbyte/callbacks.py (1)
1-7: Clean and well-organized module structure!
The imports are minimal and well-organized, and I appreciate the use of __future__ import for forward compatibility.
Adds a new docs page for the config callback feature, and a new type alias for readability.
Pairs with:
Summary by CodeRabbit
New Features
ConfigChangeCallbacktype for enhanced type safety.Bug Fixes
get_configured_catalogmethod to raise errors for invalid stream arguments.Deprecations
set_streamsandget_connectormethods as deprecated, recommending the use ofselect_streamsandget_sourceinstead.Documentation