-
Notifications
You must be signed in to change notification settings - Fork 6
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
fix: fix sorting & __init__ imports #189
Conversation
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
📝 WalkthroughWalkthroughThis pull request focuses on reorganizing and improving the import statements and Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Hey there! 👋 I noticed you've made some nice improvements to the import statements across the Airbyte CDK. A few thoughts:
The changes look solid overall - they'll definitely help improve code readability. Cheers! 🚀 Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
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: 0
🧹 Nitpick comments (4)
airbyte_cdk/sources/streams/checkpoint/__init__.py (2)
11-11
: Nicely includedResumableFullRefreshCheckpointReader
. Would you consider adding a brief docstring or comment describing its main purpose, so it’s easier for new contributors to discover and use? wdyt?
25-25
: Great thatResumableFullRefreshCursor
is now exposed. This helps clarify the module’s public interface. A brief mention in the docs might help teams properly configure or override it. wdyt?airbyte_cdk/__init__.py (2)
51-54
: Nice organization aroundconfig_observation
imports. Would you like to add a one-line docstring increate_connector_config_control_message
andemit_configuration_as_airbyte_control_message
so folks quickly understand their usage? wdyt?
84-84
: ExposingDeclarativeAuthenticator
andNoAuth
can be useful for advanced custom builds. Do you think it might be beneficial to group or annotate these authenticator classes in the docs for clarity? wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
airbyte_cdk/__init__.py
(2 hunks)airbyte_cdk/cli/source_declarative_manifest/__init__.py
(0 hunks)airbyte_cdk/models/__init__.py
(2 hunks)airbyte_cdk/sources/declarative/async_job/job_orchestrator.py
(1 hunks)airbyte_cdk/sources/declarative/auth/__init__.py
(1 hunks)airbyte_cdk/sources/declarative/decoders/__init__.py
(1 hunks)airbyte_cdk/sources/declarative/extractors/__init__.py
(1 hunks)airbyte_cdk/sources/declarative/incremental/__init__.py
(1 hunks)airbyte_cdk/sources/declarative/partition_routers/__init__.py
(1 hunks)airbyte_cdk/sources/declarative/requesters/error_handlers/__init__.py
(1 hunks)airbyte_cdk/sources/declarative/requesters/error_handlers/backoff_strategies/__init__.py
(1 hunks)airbyte_cdk/sources/declarative/requesters/paginators/__init__.py
(1 hunks)airbyte_cdk/sources/declarative/requesters/paginators/strategies/__init__.py
(1 hunks)airbyte_cdk/sources/declarative/requesters/request_options/__init__.py
(1 hunks)airbyte_cdk/sources/declarative/resolvers/__init__.py
(1 hunks)airbyte_cdk/sources/declarative/retrievers/__init__.py
(1 hunks)airbyte_cdk/sources/declarative/retrievers/async_retriever.py
(1 hunks)airbyte_cdk/sources/declarative/schema/__init__.py
(1 hunks)airbyte_cdk/sources/file_based/availability_strategy/__init__.py
(1 hunks)airbyte_cdk/sources/file_based/discovery_policy/__init__.py
(1 hunks)airbyte_cdk/sources/file_based/file_types/__init__.py
(2 hunks)airbyte_cdk/sources/file_based/schema_validation_policies/__init__.py
(1 hunks)airbyte_cdk/sources/file_based/stream/concurrent/cursor/__init__.py
(1 hunks)airbyte_cdk/sources/message/__init__.py
(1 hunks)airbyte_cdk/sources/streams/__init__.py
(1 hunks)airbyte_cdk/sources/streams/checkpoint/__init__.py
(2 hunks)airbyte_cdk/sources/streams/http/__init__.py
(1 hunks)airbyte_cdk/sources/streams/http/error_handlers/__init__.py
(2 hunks)airbyte_cdk/test/mock_http/__init__.py
(1 hunks)airbyte_cdk/test/mock_http/mocker.py
(1 hunks)airbyte_cdk/test/mock_http/response_builder.py
(1 hunks)airbyte_cdk/utils/__init__.py
(1 hunks)pyproject.toml
(0 hunks)unit_tests/sources/mock_server_tests/test_helpers/__init__.py
(1 hunks)
💤 Files with no reviewable changes (2)
- airbyte_cdk/cli/source_declarative_manifest/init.py
- pyproject.toml
✅ Files skipped from review due to trivial changes (28)
- airbyte_cdk/sources/declarative/incremental/init.py
- airbyte_cdk/sources/declarative/requesters/error_handlers/backoff_strategies/init.py
- unit_tests/sources/mock_server_tests/test_helpers/init.py
- airbyte_cdk/sources/file_based/schema_validation_policies/init.py
- airbyte_cdk/sources/declarative/retrievers/init.py
- airbyte_cdk/test/mock_http/response_builder.py
- airbyte_cdk/sources/declarative/requesters/paginators/strategies/init.py
- airbyte_cdk/sources/file_based/discovery_policy/init.py
- airbyte_cdk/sources/streams/http/init.py
- airbyte_cdk/sources/declarative/partition_routers/init.py
- airbyte_cdk/utils/init.py
- airbyte_cdk/sources/declarative/requesters/paginators/init.py
- airbyte_cdk/sources/declarative/requesters/error_handlers/init.py
- airbyte_cdk/sources/declarative/resolvers/init.py
- airbyte_cdk/sources/declarative/async_job/job_orchestrator.py
- airbyte_cdk/sources/declarative/retrievers/async_retriever.py
- airbyte_cdk/sources/streams/init.py
- airbyte_cdk/sources/message/init.py
- airbyte_cdk/sources/file_based/file_types/init.py
- airbyte_cdk/test/mock_http/init.py
- airbyte_cdk/sources/file_based/availability_strategy/init.py
- airbyte_cdk/sources/declarative/requesters/request_options/init.py
- airbyte_cdk/sources/declarative/decoders/init.py
- airbyte_cdk/sources/streams/http/error_handlers/init.py
- airbyte_cdk/test/mock_http/mocker.py
- airbyte_cdk/sources/file_based/stream/concurrent/cursor/init.py
- airbyte_cdk/sources/declarative/extractors/init.py
- airbyte_cdk/sources/declarative/schema/init.py
🔇 Additional comments (4)
airbyte_cdk/sources/declarative/auth/__init__.py (2)
6-6
: Great addition for clarity, ensuring DeclarativeOauth2Authenticator
is directly imported. Would you consider verifying there are no new circular imports if other modules import this authenticator? wdyt?
8-8
: Neat job updating the __all__
list, which makes the intended public interface more explicit. Would you consider double-checking that all references to JwtAuthenticator
and DeclarativeOauth2Authenticator
within the codebase match this final naming? wdyt?
airbyte_cdk/models/__init__.py (2)
24-31
: Reintroducing AirbyteStateStats
and AirbyteStreamStatusTraceMessage
ensures backward compatibility. Should we confirm that internal usage has been restored consistently, or do we need a note in changelogs about re-adding these classes? wdyt?
51-58
: Likewise, adding these serializers helps unify all serialization logic in the models
package. Would you consider verifying that no other modules still rely on older, removed references? wdyt?
[approve]
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
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: 0
🧹 Nitpick comments (2)
unit_tests/sources/declarative/resolvers/test_http_components_resolver.py (2)
444-444
: Consider a more concise set comprehension.
"You might simplify it by writing{stream.name for stream in actual_catalog.streams}
instead ofset([stream.name for stream in actual_catalog.streams])
. This avoids creating an unnecessary list, wdyt?"
446-446
: Keep it consistent with the previous set comprehension suggestion.
"You can also simplify this by doing{record.stream for record in records}
instead ofset([record.stream for record in records])
. Consistency across these assertions can help keep the code clean, wdyt?"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
unit_tests/sources/declarative/resolvers/test_http_components_resolver.py
(2 hunks)
🔇 Additional comments (1)
unit_tests/sources/declarative/resolvers/test_http_components_resolver.py (1)
403-403
: Great job switching from a list to a set!
"By using a set for expected_stream_names
, you ensure the test won't fail due to unexpected ordering of streams. This makes the test more robust, wdyt?"
* 'main' of https://github.com/rpopov/airbyte-python-cdk: fix: fix sorting & __init__ imports (airbytehq#189)
What
Resolve #12
Summary by CodeRabbit
DeclarativeOauth2Authenticator
to the public API.ResumableFullRefreshCheckpointReader
andResumableFullRefreshCursor
in the streams checkpoint module.__init__.py
from linting checks in the project configuration.