fix(concurrent_declarative_source): set None defaults for optional constructor args, remove unnecessary Generic implementation
#738
Conversation
…te parameter - Add default value of None to state parameter in ConcurrentDeclarativeSource.__init__() - Remove Generic[TState] from class definition as it adds no meaningful value - Update all type annotations throughout codebase to use concrete Optional[List[AirbyteStateMessage]] type - Fix test parameter order to match updated constructor signature - Resolves breaking change introduced in PR #704 where Optional state parameter lacked default value Fixes: #704 Co-Authored-By: AJ Steers <[email protected]>
Original prompt from AJ Steers |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Testing This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@devin/1735434900-fix-optional-state-parameter#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch devin/1735434900-fix-optional-state-parameterHelpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a breaking change in ConcurrentDeclarativeSource.__init__() where the state parameter was typed as optional but lacked a default value, causing failures when called with state=None.
Key changes:
- Added default value
= Noneto thestateparameter in the constructor - Reordered constructor parameters to move
stateaftersource_config(required for Python syntax) - Removed
Generic[TState]from class definition and updated type annotations across files
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| airbyte_cdk/sources/declarative/concurrent_declarative_source.py | Updated constructor signature with default value and parameter reordering |
| airbyte_cdk/sources/declarative/yaml_declarative_source.py | Removed generic type annotation |
| airbyte_cdk/manifest_server/routers/manifest.py | Updated return type annotation |
| airbyte_cdk/manifest_server/command_processor/utils.py | Updated return type annotation |
| airbyte_cdk/manifest_server/command_processor/processor.py | Updated type annotations for class field and constructor |
| airbyte_cdk/connector_builder/test_reader/reader.py | Updated method parameter type annotations |
| airbyte_cdk/connector_builder/main.py | Updated function parameter type annotation |
| airbyte_cdk/connector_builder/connector_builder_handler.py | Updated function signatures and return type annotations |
| unit_tests/legacy/sources/declarative/test_manifest_declarative_source.py | Updated constructor call to match new parameter order |
Comments suppressed due to low confidence (1)
airbyte_cdk/manifest_server/command_processor/utils.py:1
- The constructor call uses the old parameter order. After the PR changes,
source_configshould come beforestate. This should be:ConcurrentDeclarativeSource(catalog=catalog, config=config, source_config=definition, state=state, ...)
import copy
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
📝 WalkthroughWalkthroughRemoved generic typing from ConcurrentDeclarativeSource and simplified/expanded its constructor signature (made catalog/config/state optional, added keyword-only params including source_config, normalize_manifest, limits, config_path). Updated type annotations and call sites across connector-builder, manifest server, YAML source, and tests; runtime control flow unchanged. Changes
Sequence Diagram(s)(omitted — changes are type/constructor-signature only; no control-flow modifications to diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
airbyte_cdk/sources/declarative/yaml_declarative_source.py (1)
35-41: Preserve None semantics for state and forward debug to the base, wdyt?To fully realize the PR’s goal (accepting state=None) and to respect the new debug parameter on the base init, could we stop coercing None to [] and pass debug through?
Apply this diff:
- super().__init__( - catalog=catalog or ConfiguredAirbyteCatalog(streams=[]), - config=config or {}, - state=state or [], - source_config=source_config, - config_path=config_path, - ) + super().__init__( + catalog=catalog or ConfiguredAirbyteCatalog(streams=[]), + config=config or {}, + source_config=source_config, + state=state, + debug=debug, + config_path=config_path, + )airbyte_cdk/sources/declarative/concurrent_declarative_source.py (2)
170-183: Add a backward-compat shim for positional callers to avoid breakage.Reordering
source_configbeforestateis the right API, but any positional call sites using the previous order(catalog, config, state, source_config)will now misbind and crash. Would you add a small compatibility swap with a deprecation log so external callers don’t break, wdyt?Apply this diff (in init just after the signature and before using these params):
@@ def __init__( self, catalog: Optional[ConfiguredAirbyteCatalog], config: Optional[Mapping[str, Any]], source_config: ConnectionDefinition, state: Optional[List[AirbyteStateMessage]] = None, debug: bool = False, emit_connector_builder_messages: bool = False, migrate_manifest: bool = False, normalize_manifest: bool = False, limits: Optional[TestLimits] = None, config_path: Optional[str] = None, **kwargs: Any, ) -> None: + # Back-compat for legacy positional ordering: (catalog, config, state, source_config) + # If we detect a list where source_config should be, and a Mapping where state should be, swap them. + if isinstance(source_config, list) and (state is None or isinstance(state, Mapping)): + logging.getLogger("airbyte.cdk").warning( + "Detected legacy positional args to ConcurrentDeclarativeSource(...). " + "Please pass source_config before state; accepting old order for now." + ) + from typing import cast + legacy_state = cast(Optional[List[AirbyteStateMessage]], source_config) + legacy_source_config = cast(ConnectionDefinition, state or {}) + source_config, state = legacy_source_config, legacy_stateThis keeps runtime compatibility while nudging callers to update.
170-183: Convert legacy positional call to keyword args & verify wrapper
- unit_tests/legacy/sources/declarative/test_manifest_declarative_source.py:2222 uses
ConcurrentDeclarativeSourcewith 4 positional args (nostate=/source_config=)—please update to explicit keywords to match the new__init__signature.- No generic-type subscripts (
ConcurrentDeclarativeSource[…]) remain.- In
manifest_server/command_processor/utils.py,build_sourcedefines the wrapper—can you confirm it instantiatesConcurrentDeclarativeSourcewithsource_config=andstate=parameters? wdyt?
🧹 Nitpick comments (5)
airbyte_cdk/manifest_server/command_processor/processor.py (1)
33-35: Consider widening state to Optional to mirror source.read() expectations, wdyt?The method type hints state: List[AirbyteStateMessage], but upstream can legitimately provide None now.
- state: List[AirbyteStateMessage], + state: Optional[List[AirbyteStateMessage]],airbyte_cdk/connector_builder/main.py (1)
72-79: Nit: align state annotation with Optional to reflect accepted None, wdyt?This keeps the handler consistent with create_source/read_stream flows that tolerate None.
-def handle_connector_builder_request( - source: ConcurrentDeclarativeSource, - command: str, - config: Mapping[str, Any], - catalog: Optional[ConfiguredAirbyteCatalog], - state: List[AirbyteStateMessage], - limits: TestLimits, +def handle_connector_builder_request( + source: ConcurrentDeclarativeSource, + command: str, + config: Mapping[str, Any], + catalog: Optional[ConfiguredAirbyteCatalog], + state: Optional[List[AirbyteStateMessage]], + limits: TestLimits,airbyte_cdk/manifest_server/routers/manifest.py (1)
43-45: Return type change to non-generic looks right.This aligns with removing Generic[TState] across the codebase. As a minor hardening, would you consider calling
build_sourcewith keyword args to guard against any future parameter reordering there, if supported by its signature (e.g.,build_source(manifest_dict=..., catalog=..., config_dict=..., state=..., record_limit=..., page_limit=..., slice_limit=...)), wdyt?airbyte_cdk/sources/declarative/concurrent_declarative_source.py (1)
165-165: Dropping Generic[TState] from the class is a good simplification.Optionally, since the class is no longer generic, would you like to remove the now-unused generic-related imports (e.g.,
Generic, possiblyTState) if linters allow it, to keep things tidy, wdyt?airbyte_cdk/connector_builder/connector_builder_handler.py (1)
63-69: Return type change to non-generic looks good.Tiny readability nit: in the call below, would you like to order the keyword args to match the new constructor (put
source_config=beforestate=) to reduce cognitive load for readers, wdyt?- return ConcurrentDeclarativeSource( - catalog=catalog, - config=config, - state=state, - source_config=manifest, + return ConcurrentDeclarativeSource( + catalog=catalog, + config=config, + source_config=manifest, + state=state, emit_connector_builder_messages=True, migrate_manifest=should_migrate_manifest(config), normalize_manifest=should_normalize_manifest(config), limits=limits, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
airbyte_cdk/connector_builder/connector_builder_handler.py(4 hunks)airbyte_cdk/connector_builder/main.py(1 hunks)airbyte_cdk/connector_builder/test_reader/reader.py(2 hunks)airbyte_cdk/manifest_server/command_processor/processor.py(1 hunks)airbyte_cdk/manifest_server/command_processor/utils.py(1 hunks)airbyte_cdk/manifest_server/routers/manifest.py(1 hunks)airbyte_cdk/sources/declarative/concurrent_declarative_source.py(2 hunks)airbyte_cdk/sources/declarative/yaml_declarative_source.py(1 hunks)unit_tests/legacy/sources/declarative/test_manifest_declarative_source.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-18T23:40:06.391Z
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
Applied to files:
airbyte_cdk/sources/declarative/yaml_declarative_source.pyunit_tests/legacy/sources/declarative/test_manifest_declarative_source.py
📚 Learning: 2024-11-15T01:04:21.272Z
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/cli/source_declarative_manifest/_run.py:62-65
Timestamp: 2024-11-15T01:04:21.272Z
Learning: The files in `airbyte_cdk/cli/source_declarative_manifest/`, including `_run.py`, are imported from another repository, and changes to these files should be minimized or avoided when possible to maintain consistency.
Applied to files:
unit_tests/legacy/sources/declarative/test_manifest_declarative_source.py
🧬 Code graph analysis (8)
airbyte_cdk/sources/declarative/yaml_declarative_source.py (1)
airbyte_cdk/sources/declarative/concurrent_declarative_source.py (1)
ConcurrentDeclarativeSource(165-1057)
airbyte_cdk/manifest_server/command_processor/utils.py (1)
airbyte_cdk/sources/declarative/concurrent_declarative_source.py (1)
ConcurrentDeclarativeSource(165-1057)
airbyte_cdk/manifest_server/routers/manifest.py (1)
airbyte_cdk/sources/declarative/concurrent_declarative_source.py (1)
ConcurrentDeclarativeSource(165-1057)
airbyte_cdk/connector_builder/connector_builder_handler.py (1)
airbyte_cdk/sources/declarative/concurrent_declarative_source.py (2)
ConcurrentDeclarativeSource(165-1057)TestLimits(126-137)
airbyte_cdk/connector_builder/test_reader/reader.py (1)
airbyte_cdk/sources/declarative/concurrent_declarative_source.py (1)
ConcurrentDeclarativeSource(165-1057)
airbyte_cdk/connector_builder/main.py (1)
airbyte_cdk/sources/declarative/concurrent_declarative_source.py (1)
ConcurrentDeclarativeSource(165-1057)
airbyte_cdk/manifest_server/command_processor/processor.py (1)
airbyte_cdk/sources/declarative/concurrent_declarative_source.py (1)
ConcurrentDeclarativeSource(165-1057)
airbyte_cdk/sources/declarative/concurrent_declarative_source.py (1)
airbyte_cdk/models/airbyte_protocol.py (1)
AirbyteStateMessage(67-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: source-intercom
- GitHub Check: Check: destination-motherduck
- GitHub Check: Check: source-shopify
- GitHub Check: SDM Docker Image Build
- GitHub Check: Manifest Server Docker Image Build
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.13, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Analyze (python)
🔇 Additional comments (9)
airbyte_cdk/sources/declarative/yaml_declarative_source.py (1)
17-17: LGTM: drop of generics in base class is consistent with the PR.Extending the non-generic ConcurrentDeclarativeSource aligns with the refactor and avoids needless type noise. Nice.
airbyte_cdk/manifest_server/command_processor/processor.py (1)
24-24: LGTM: non-generic type annotations.The _source attribute and constructor parameter now reference the concrete ConcurrentDeclarativeSource, matching the refactor.
Also applies to: 27-27
airbyte_cdk/manifest_server/command_processor/utils.py (1)
66-66: LGTM: return type updated to concrete ConcurrentDeclarativeSource.Signature matches the refactor; body already uses keyword args aligned to the reordered constructor.
airbyte_cdk/connector_builder/main.py (1)
72-72: LGTM: non-generic source parameter.The handler signature now matches the concrete type used across the codebase.
airbyte_cdk/connector_builder/test_reader/reader.py (2)
86-94: Type hint update (non-generic ConcurrentDeclarativeSource) looks good.Consistent with the class losing its generic. No further changes needed here, wdyt?
384-391: Type hint update in _read_stream is correct.Matches the new non-generic source type; no functional impact. All good, wdyt?
airbyte_cdk/connector_builder/connector_builder_handler.py (3)
91-96: Type hint update for read_stream’s source param is correct.Matches the non-generic class; no issues, wdyt?
129-132: Type hint update for resolve_manifest is correct.No functional changes; looks good, wdyt?
149-150: Type hint update for full_resolve_manifest is correct.Consistent with the rest of the refactor, wdyt?
unit_tests/legacy/sources/declarative/test_manifest_declarative_source.py
Outdated
Show resolved
Hide resolved
unit_tests/legacy/sources/declarative/test_manifest_declarative_source.py
Outdated
Show resolved
Hide resolved
…r call - Address GitHub PR comment by @aaronsteers requesting explicit parameter names - Improve code readability by using named arguments instead of positional arguments - Apply automatic formatting fixes to maintain code style consistency Co-Authored-By: AJ Steers <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
unit_tests/sources/declarative/interpolation/test_interpolated_string.py (1)
65-67: Prefer isinstance over direct type equality (and keep it single-line).Comparing exact types is brittle and the new multi-line formatting adds noise. Using isinstance(val, float) is clearer and future-proof. Also, to reduce flakiness with float comparisons, consider pytest.approx for the numeric equality check above (Line 61). Wdyt?
Apply this diff:
- assert float == type( - val - ), f"Expected float, got {type(val)} for value {val} in test {test_name}" + assert isinstance(val, float), f"Expected float, got {type(val)} for value {val} in test {test_name}"Optionally (outside this hunk), you could make the equality more robust:
# at Line 61 from pytest import approx # top of file (optional) # replace: assert val == expected_value # with: assert (val == expected_value) if expected_value is None else (val == pytest.approx(expected_value))unit_tests/test_entrypoint.py (1)
833-835: Add a guard for missing sourceStats to improve failure clarity?If
sourceStatsorrecordCountwere ever absent, this would raise an AttributeError instead of a clear assertion failure. Would you consider asserting their presence first, then checking type, to yield better error messages, wdyt?- assert isinstance( - actual_message.state.sourceStats.recordCount, float - ), "recordCount value should be expressed as a float" + # Fail fast if sourceStats or recordCount is missing before type check + assert getattr(actual_message.state, "sourceStats", None) is not None, "STATE messages should include sourceStats" + assert hasattr(actual_message.state.sourceStats, "recordCount"), "STATE.sourceStats must include recordCount" + assert isinstance(actual_message.state.sourceStats.recordCount, float), "recordCount value should be expressed as a float"unit_tests/sources/declarative/parsers/test_model_to_component_factory.py (1)
3440-3442: Polish the assertion message grammar.The consolidated equality check is fine; could we tweak the failure text for clarity?
- ), "State migration was called, but actual state don't match expected" + ), "State migration was called, but the actual state doesn't match the expected value"Wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (23)
airbyte_cdk/connector_builder/main.py(2 hunks)airbyte_cdk/sources/concurrent_source/concurrent_source.py(1 hunks)airbyte_cdk/sources/file_based/file_based_source.py(1 hunks)airbyte_cdk/sources/file_based/stream/concurrent/adapters.py(1 hunks)airbyte_cdk/sql/shared/sql_processor.py(1 hunks)unit_tests/connector_builder/test_connector_builder_handler.py(2 hunks)unit_tests/destinations/test_destination.py(1 hunks)unit_tests/legacy/sources/declarative/partition_routers/test_parent_state_stream.py(2 hunks)unit_tests/legacy/sources/declarative/test_manifest_declarative_source.py(1 hunks)unit_tests/source_declarative_manifest/test_source_declarative_w_custom_components.py(1 hunks)unit_tests/sources/declarative/incremental/test_concurrent_perpartitioncursor.py(3 hunks)unit_tests/sources/declarative/interpolation/test_interpolated_string.py(1 hunks)unit_tests/sources/declarative/parsers/test_model_to_component_factory.py(1 hunks)unit_tests/sources/declarative/partition_routers/test_grouping_partition_router.py(3 hunks)unit_tests/sources/declarative/partition_routers/test_substream_partition_router.py(1 hunks)unit_tests/sources/declarative/test_concurrent_declarative_source.py(1 hunks)unit_tests/sources/file_based/test_scenarios.py(2 hunks)unit_tests/sources/streams/test_call_rate.py(2 hunks)unit_tests/sources/streams/test_stream_read.py(1 hunks)unit_tests/test_entrypoint.py(1 hunks)unit_tests/test_exception_handler.py(1 hunks)unit_tests/test_secure_logger.py(2 hunks)unit_tests/utils/test_secret_utils.py(1 hunks)
✅ Files skipped from review due to trivial changes (16)
- unit_tests/sources/streams/test_stream_read.py
- unit_tests/sources/file_based/test_scenarios.py
- unit_tests/destinations/test_destination.py
- airbyte_cdk/sources/concurrent_source/concurrent_source.py
- unit_tests/connector_builder/test_connector_builder_handler.py
- unit_tests/sources/declarative/partition_routers/test_grouping_partition_router.py
- unit_tests/legacy/sources/declarative/partition_routers/test_parent_state_stream.py
- unit_tests/test_secure_logger.py
- unit_tests/utils/test_secret_utils.py
- unit_tests/source_declarative_manifest/test_source_declarative_w_custom_components.py
- unit_tests/sources/declarative/partition_routers/test_substream_partition_router.py
- unit_tests/sources/declarative/test_concurrent_declarative_source.py
- airbyte_cdk/sources/file_based/stream/concurrent/adapters.py
- unit_tests/sources/declarative/incremental/test_concurrent_perpartitioncursor.py
- unit_tests/test_exception_handler.py
- unit_tests/sources/streams/test_call_rate.py
🚧 Files skipped from review as they are similar to previous changes (1)
- unit_tests/legacy/sources/declarative/test_manifest_declarative_source.py
🧰 Additional context used
🧬 Code graph analysis (1)
airbyte_cdk/connector_builder/main.py (1)
airbyte_cdk/sources/declarative/concurrent_declarative_source.py (1)
ConcurrentDeclarativeSource(165-1057)
🪛 GitHub Actions: Linters
airbyte_cdk/sources/file_based/file_based_source.py
[warning] 282-290: Ruff format change detected: multi-line assert reformatted to single-line form.
airbyte_cdk/connector_builder/main.py
[warning] 80-88: Ruff format change detected: multi-line assert in test_read was reformatted to a single-line assertion. Apply the formatting changes.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Check: source-intercom
- GitHub Check: Check: destination-motherduck
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: source-google-drive
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Check: source-shopify
- GitHub Check: Pytest (Fast)
- GitHub Check: Manifest Server Docker Image Build
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.13, Ubuntu)
🔇 Additional comments (1)
airbyte_cdk/connector_builder/main.py (1)
73-79: Signature switch to non-generic ConcurrentDeclarativeSource looks right.This aligns with removing the generic across the codebase. LGTM.
Co-Authored-By: AJ Steers <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
airbyte_cdk/connector_builder/main.py (1)
72-79: Makestateparameter Optional to reflect new semantics.Constructor now accepts
state: Optional[List[AirbyteStateMessage]] = None; this handler should mirror that to avoid type drift and ease passing throughNone.def handle_connector_builder_request( source: ConcurrentDeclarativeSource, command: str, config: Mapping[str, Any], catalog: Optional[ConfiguredAirbyteCatalog], - state: List[AirbyteStateMessage], + state: Optional[List[AirbyteStateMessage]], limits: TestLimits, ) -> AirbyteMessage:Wdyt?
♻️ Duplicate comments (1)
airbyte_cdk/connector_builder/main.py (1)
83-85: Replace assert with explicit guard for CLI validation.Asserts can be stripped with optimizations; a clear exception is safer for users invoking
test_read.- assert catalog is not None, ( - "`test_read` requires a valid `ConfiguredAirbyteCatalog`, got None." - ) + if catalog is None: + raise ValueError("`test_read` requires a valid `ConfiguredAirbyteCatalog`, got None.")Wdyt?
🧹 Nitpick comments (2)
airbyte_cdk/connector_builder/main.py (2)
61-63: PropagateNonewhen no state is provided.To align with the fixed constructor that treats missing state as
None, consider returningstate=Nonehere instead of an empty list.- catalog = None - state = [] + catalog = None + state = NoneWdyt?
35-38: Tighten return type forstatein helper’s signature.Use
Optional[List[AirbyteStateMessage]]instead ofAnyfor stronger typing and consistency.) -> Tuple[str, Mapping[str, Any], Optional[ConfiguredAirbyteCatalog], Any]: + ) -> Tuple[str, Mapping[str, Any], Optional[ConfiguredAirbyteCatalog], Optional[List[AirbyteStateMessage]]]:Wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
airbyte_cdk/connector_builder/main.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
airbyte_cdk/connector_builder/main.py (1)
airbyte_cdk/sources/declarative/concurrent_declarative_source.py (1)
ConcurrentDeclarativeSource(165-1057)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Check: source-intercom
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Check: destination-motherduck
- GitHub Check: Check: source-shopify
- GitHub Check: SDM Docker Image Build
- GitHub Check: Manifest Server Docker Image Build
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (All, Python 3.13, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Analyze (python)
…ce constructor in test Co-Authored-By: AJ Steers <[email protected]>
aaronsteers
changed the title
None defaults for optional constructor args
aaronsteers
changed the title
None defaults for optional constructor argsNone defaults for optional constructor args, remove unnecessary Generic implementation
airbyte_cdk/sources/declarative/concurrent_declarative_source.py
Outdated
Show resolved
Hide resolved
…y for correctness
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
airbyte_cdk/sources/declarative/concurrent_declarative_source.py (4)
170-184: Makestatekeyword-only as well to prevent accidental positional misuse, wdyt?
Placing*right afterconfigforces all subsequent params to be passed as keywords. This avoids old callers accidentally binding a manifest tostatepositionally and gives a clearer error.Apply:
def __init__( self, catalog: Optional[ConfiguredAirbyteCatalog], config: Optional[Mapping[str, Any]], - state: Optional[List[AirbyteStateMessage]] = None, - *, - source_config: ConnectionDefinition, + *, + source_config: ConnectionDefinition, + state: Optional[List[AirbyteStateMessage]] = None, debug: bool = False, emit_connector_builder_messages: bool = False, migrate_manifest: bool = False, normalize_manifest: bool = False, limits: Optional[TestLimits] = None, config_path: Optional[str] = None, **kwargs: Any, ) -> None:
185-189: Fail noisily (or at least warn) on unexpected kwargs, wdyt?
**kwargscan mask typos. Consider logging or raising to help users.self.logger = logging.getLogger(f"airbyte.{self.name}") + # Surface unexpected keyword-only args early to prevent silent misconfigurations + if kwargs: + self.logger.warning("Ignoring unexpected keyword-only args: %s", sorted(kwargs.keys())) + self._limits = limits
189-193: Drop the# type: ignoreby tightening ConnectorStateManager typing, wdyt?
If feasible, updatingConnectorStateManagerto acceptOptional[List[AirbyteStateMessage]]would remove the need for# type: ignorehere and improve type safety downstream.
291-296: Fix docstring: return type isn’tNone, wdyt?
_pre_process_manifestreturns a dict.- Returns: - None + Returns: + Dict[str, Any]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/concurrent_declarative_source.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
🧬 Code graph analysis (1)
airbyte_cdk/sources/declarative/concurrent_declarative_source.py (1)
airbyte_cdk/models/airbyte_protocol.py (1)
AirbyteStateMessage(67-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: source-intercom
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Check: destination-motherduck
- GitHub Check: Check: source-shopify
- GitHub Check: Manifest Server Docker Image Build
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.13, Ubuntu)
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
airbyte_cdk/sources/declarative/concurrent_declarative_source.py (2)
165-165: LGTM: removing Generic from ConcurrentDeclarativeSource simplifies the public API.
This aligns with actual usage and avoids misleading type parameters.
170-184: No remaining positional constructor calls detected. Verified that all ConcurrentDeclarativeSource calls use thesource_configkeyword and at most two positional args—ready to resolve, wdyt?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/concurrent_declarative_source.py (1)
170-176: Consider enforcing keyword-only args for all ctor parameters and synchronizing PR summary
- Since all existing calls already supply
source_configas a keyword (no positional usage detected), makingcatalog,config, andstatekeyword-only by placing*immediately afterselfis optional but can help prevent future misbindings—could you adjust the signature to:wdyt?def __init__(self, *, source_config: ConnectionDefinition, catalog: Optional[ConfiguredAirbyteCatalog] = None, config: Optional[Mapping[str, Any]] = None, state: Optional[List[AirbyteStateMessage]] = None, debug: bool = False, …)- The PR description mentions moving
stateaftersource_config, but the signature still listsstatebefore the barrier—would you update the PR text to match the final code?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/concurrent_declarative_source.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
📚 Learning: 2024-11-18T23:40:06.391Z
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
Applied to files:
airbyte_cdk/sources/declarative/concurrent_declarative_source.py
🧬 Code graph analysis (1)
airbyte_cdk/sources/declarative/concurrent_declarative_source.py (2)
airbyte_cdk/sources/abstract_source.py (1)
AbstractSource(52-326)airbyte_cdk/models/airbyte_protocol.py (1)
AirbyteStateMessage(67-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Check: source-shopify
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.13, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
🔇 Additional comments (1)
airbyte_cdk/sources/declarative/concurrent_declarative_source.py (1)
165-165: Removing Generic[TState] from ConcurrentDeclarativeSource looks goodThis aligns with the concrete state usage throughout and simplifies the API surface, nice.
…gs to resolve MyPy error Co-Authored-By: AJ Steers <[email protected]>
…://git-manager.devin.ai/proxy/github.com/airbytehq/airbyte-python-cdk into devin/1735434900-fix-optional-state-parameter
…h ConcurrentDeclarativeSource signature Co-Authored-By: AJ Steers <[email protected]>
fix(concurrent_declarative_source): add default value to optional state parameter
Summary
Fixes a breaking change introduced in PR #704 where the
stateparameter inConcurrentDeclarativeSource.__init__()was typed asOptional[List[AirbyteStateMessage]]but lacked a default value, causing failures whencreate_source()was called with (implicit)state=None.Key changes:
= Nonedefault value to thestateparameterstateaftersource_config(required for Python syntax)Generic[TState]from class definition as it provided no meaningful value (always the same concrete type)ConcurrentDeclarativeSourcetypeReview & Testing Checklist for Human
ConcurrentDeclarativeSourceconstructor are broken - The parameter reordering from(catalog, config, state, source_config)to(catalog, config, source_config, state=None)could break existing codecreate_source()with minimal arguments - Ensurecreate_source(config=config, limits=limits, catalog=None, state=None)works without errorsGeneric[TState]changes the public API surface and could affect external CDK consumersNotes
test_create_source()intest_connector_builder_handler.pyalready validates the minimal argument case and should now passSummary by CodeRabbit
New Features
Refactor
Tests
@CodeRabbit ignore