airbyte_cdk/manifest_server/command_processor/utils.py:1

• The constructor call uses the old parameter order. After the PR changes, source_config should come before state. This should be: ConcurrentDeclarativeSource(catalog=catalog, config=config, source_config=definition, state=state, ...)

import copy

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_source with 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, possibly TState) 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= before state=) 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,
     )

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.

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.

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.

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 sourceStats or recordCount were 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?

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.

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?

airbyte_cdk/connector_builder/main.py (2)

61-63: Propagate None when no state is provided.

To align with the fixed constructor that treats missing state as None, consider returning state=None here instead of an empty list.

-        catalog = None
-        state = []
+        catalog = None
+        state = None

Wdyt?

---

35-38: Tighten return type for state in helper’s signature.

Use Optional[List[AirbyteStateMessage]] instead of Any for stronger typing and consistency.

 ) -> Tuple[str, Mapping[str, Any], Optional[ConfiguredAirbyteCatalog], Any]:
+ ) -> Tuple[str, Mapping[str, Any], Optional[ConfiguredAirbyteCatalog], Optional[List[AirbyteStateMessage]]]:

Wdyt?

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.

airbyte_cdk/sources/declarative/concurrent_declarative_source.py (4)

170-184: Make state keyword-only as well to prevent accidental positional misuse, wdyt?
Placing * right after config forces all subsequent params to be passed as keywords. This avoids old callers accidentally binding a manifest to state positionally 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?
**kwargs can 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: ignore by tightening ConnectorStateManager typing, wdyt?
If feasible, updating ConnectorStateManager to accept Optional[List[AirbyteStateMessage]] would remove the need for # type: ignore here and improve type safety downstream.

---

291-296: Fix docstring: return type isn’t None, wdyt?
_pre_process_manifest returns a dict.

-        Returns:
-            None
+        Returns:
+            Dict[str, Any]

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.

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.

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_config as a keyword (no positional usage detected), making catalog, config, and state keyword-only by placing * immediately after self is optional but can help prevent future misbindings—could you adjust the signature to:

   def __init__(self, *, source_config: ConnectionDefinition, catalog: Optional[ConfiguredAirbyteCatalog] = None, config: Optional[Mapping[str, Any]] = None, state: Optional[List[AirbyteStateMessage]] = None, debug: bool = False, …)

  wdyt?
• The PR description mentions moving state after source_config, but the signature still lists state before the barrier—would you update the PR text to match the final code?

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.

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.

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.