Skip to content

Fix: Instead of failing, raise a warning if stream selection is performed before config is present#285

Merged
Aaron ("AJ") Steers (aaronsteers) merged 18 commits into
airbytehq:mainfrom
Suraj-Vishwakarma70:fxx
Jul 9, 2024
Merged

Fix: Instead of failing, raise a warning if stream selection is performed before config is present#285
Aaron ("AJ") Steers (aaronsteers) merged 18 commits into
airbytehq:mainfrom
Suraj-Vishwakarma70:fxx

Conversation

@Suraj-Vishwakarma70

@Suraj-Vishwakarma70 Suraj (Suraj-Vishwakarma70) commented Jul 1, 2024

Copy link
Copy Markdown
Contributor

This fixes #51
Aaron ("AJ") Steers (@aaronsteers) ... Let me know if that doesnt work!

Summary by CodeRabbit

  • New Features

    • Introduced logging messages to enhance visibility of stream selection and availability issues.
    • Improved handling for cases with missing configurations or no available streams.
  • Improvements

    • Enhanced configuration management and stream selection to address pending and incremental stream scenarios.

@coderabbitai

coderabbitai Bot commented Jul 1, 2024

Copy link
Copy Markdown
Contributor
Walkthrough

Walkthrough

The updates to airbyte/sources/base.py enhance the handling of stream configurations by introducing logging messages and logic to manage cases where streams or configurations might be missing. New attributes and methods have been added to the Source class to improve the robustness of stream selection and availability checks. These changes primarily address an issue where calling select_streams before set_config led to unexpected failures.

Changes

Files Change Summary
airbyte/sources/base.py Added and modified attributes, methods, and logging logic in the Source class to handle missing streams and configurations.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Source
    participant Logger

    User->>Source: select_streams()
    Source->>Logger: Log warning/config check
    alt Configuration missing
        Source->>Logger: Log missing configuration
        Source->>User: Inform missing config
    else Configuration present
        Source->>Source: Get available streams
        Source->>Logger: Log streams handling
        alt Streams missing
            Source->>Logger: Log missing streams
            Source->>User: Inform missing streams
        end
        Source->User: Return streams
    end
Loading

Assessment against linked issues

Objective Addressed Explanation
Handle select_streams() before set_config() without raising a failure (#51)
Print warning if streams can't be validated due to missing configuration instead of hard failure (#51)

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@aaronsteers

Copy link
Copy Markdown
Member

CodeRabbit (@coderabbitai) review

@coderabbitai

coderabbitai Bot commented Jul 1, 2024

Copy link
Copy Markdown
Contributor
Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between cb0153e and 9143db6.

Files selected for processing (1)
  • airbyte/sources/base.py (3 hunks)
Additional context used
Ruff
airbyte/sources/base.py

129-129: Line too long (116 > 100)

(E501)

Additional comments not posted (1)
airbyte/sources/base.py (1)

227-229: LGTM!

The logging functionality added to handle missing configuration is correct.

Comment thread airbyte/sources/base.py Outdated
@Suraj-Vishwakarma70

Copy link
Copy Markdown
Contributor Author

Aaron ("AJ") Steers (@aaronsteers) I've made one more commit. I got a better approach to solve this! I gues you'll need to run the tests again. Thanks

Comment thread airbyte/sources/base.py Outdated
@aaronsteers Aaron ("AJ") Steers (aaronsteers) changed the title A warning is now raised if config is not present Fix: Raise a warning is if config is not present Jul 8, 2024
@aaronsteers Aaron ("AJ") Steers (aaronsteers) changed the title Fix: Raise a warning is if config is not present Fix: Raise a warning if config is not present Jul 8, 2024
@aaronsteers

Aaron ("AJ") Steers (aaronsteers) commented Jul 8, 2024

Copy link
Copy Markdown
Member

/fix-pr

Auto-Fix Job Info

This job attempts to auto-fix any linting or formating issues. If any fixes are made,
those changes will be automatically committed and pushed back to the PR.
(This job requires that the PR author has "Allow edits from maintainers" enabled.)

PR auto-fix job started... Check job output.

✅ Changes applied successfully.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 9143db6 and b7a3994.

Files selected for processing (1)
  • airbyte/sources/base.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
  • airbyte/sources/base.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range and nitpick comments (4)
run_example.py (3)

1-2: Add missing required import and copyright notice.

Add the following import at the top to ensure forward compatibility and a copyright notice.

from __future__ import annotations

# Copyright (c) 2023 Airbyte, Inc., all rights reserved.
Tools
Ruff

1-1: Missing required import: from __future__ import annotations

Insert required import: from __future__ import annotations

(I002)


1-1: Missing copyright notice at top of file

(CPY001)


13-27: Add missing return type annotations for public methods.

Add return type annotations to all public methods in the MockExecutor class.

- def ensure_installation(self, auto_fix: bool):
+ def ensure_installation(self, auto_fix: bool) -> None:

- def get_installed_version(self):
+ def get_installed_version(self) -> str:

- def install(self):
+ def install(self) -> None:

- def uninstall(self):
+ def uninstall(self) -> None:

- def execute(self, args):
+ def execute(self, args: list[str]) -> list[str]:
Tools
Ruff

13-13: Expected 2 blank lines, found 1

Add missing blank line(s)

(E302)


14-14: Missing return type annotation for public function ensure_installation

Add return type annotation: None

(ANN201)


14-14: Boolean-typed positional argument in function definition

(FBT001)


17-17: Missing return type annotation for public function get_installed_version

Add return type annotation: str

(ANN201)


20-20: Missing return type annotation for public function install

Add return type annotation: None

(ANN201)


23-23: Missing return type annotation for public function uninstall

Add return type annotation: None

(ANN201)


26-26: Missing return type annotation for public function execute

(ANN201)


26-26: Missing type annotation for function argument args

(ANN001)


26-26: Unused method argument: args

(ARG002)


30-44: Add missing return type annotation and blank line.

Add a return type annotation to the run_example function and a blank line after the function definition.

- def run_example():
+ def run_example() -> None:

- if __name__ == "__main__":
+    if __name__ == "__main__":
Tools
Ruff

30-30: Expected 2 blank lines, found 1

Add missing blank line(s)

(E302)


30-30: Missing return type annotation for public function run_example

Add return type annotation: None

(ANN201)


44-44: Expected 2 blank lines after class or function definition, found (1)

Add missing blank line(s)

(E305)

airbyte/sources/base.py (1)

121-123: Fix line length issue.

The line exceeds the maximum length of 100 characters.

- print("Warning : Configuration is missing. Streams will be selected after configuration is set.")
+ print("Warning: Configuration is missing. Streams will be selected after configuration is set.")
Tools
Ruff

122-122: Line too long (109 > 100)

(E501)

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between b7a3994 and 1f80494.

Files selected for processing (2)
  • airbyte/sources/base.py (12 hunks)
  • run_example.py (1 hunks)
Additional context used
Ruff
run_example.py

1-1: Missing required import: from __future__ import annotations

Insert required import: from __future__ import annotations

(I002)


1-1: Missing copyright notice at top of file

(CPY001)


13-13: Expected 2 blank lines, found 1

Add missing blank line(s)

(E302)


14-14: Missing return type annotation for public function ensure_installation

Add return type annotation: None

(ANN201)


14-14: Boolean-typed positional argument in function definition

(FBT001)


17-17: Missing return type annotation for public function get_installed_version

Add return type annotation: str

(ANN201)


20-20: Missing return type annotation for public function install

Add return type annotation: None

(ANN201)


23-23: Missing return type annotation for public function uninstall

Add return type annotation: None

(ANN201)


26-26: Missing return type annotation for public function execute

(ANN201)


26-26: Missing type annotation for function argument args

(ANN001)


26-26: Unused method argument: args

(ARG002)


30-30: Expected 2 blank lines, found 1

Add missing blank line(s)

(E302)


30-30: Missing return type annotation for public function run_example

Add return type annotation: None

(ANN201)


44-44: Expected 2 blank lines after class or function definition, found (1)

Add missing blank line(s)

(E305)

airbyte/sources/base.py

85-85: Trailing whitespace

Remove trailing whitespace

(W291)


122-122: Line too long (109 > 100)

(E501)


124-124: Blank line contains whitespace

Remove whitespace from blank line

(W293)


174-174: Blank line contains whitespace

Remove whitespace from blank line

(W293)


205-205: Blank line contains whitespace

Remove whitespace from blank line

(W293)


215-215: Too many blank lines (2)

Remove extraneous blank line(s)

(E303)


829-829: No newline at end of file

Add trailing newline

(W292)

Additional comments not posted (10)
airbyte/sources/base.py (10)

Line range hint 1-8:
LGTM!

The import statements are comprehensive and correctly used.


79-85: LGTM!

The new attributes _stream_names_observed and _pending_streams are correctly initialized.

Tools
Ruff

85-85: Trailing whitespace

Remove trailing whitespace

(W291)


135-139: LGTM!

The logging messages for missing configurations are correctly implemented.


174-178: LGTM!

The logic for handling pending streams is correctly implemented.

Tools
Ruff

174-174: Blank line contains whitespace

Remove whitespace from blank line

(W293)


187-188: LGTM!

The logging message for missing configurations is correctly implemented.


200-212: LGTM!

The logging messages for missing configurations and catalogs are correctly implemented.

Tools
Ruff

205-205: Blank line contains whitespace

Remove whitespace from blank line

(W293)


247-249: LGTM!

The logging message for missing configurations is correctly implemented.


788-798: LGTM!

The logic for handling incremental streams is correctly implemented.


662-674: LGTM!

The _log_incremental_streams method is correctly implemented and provides meaningful information.


675-677: LGTM!

The _log_stream_read_start method is correctly implemented and provides meaningful information.

Latest Changes

1)Added pending streams to handle streams when config isnt present
2)Ensured Warning message when config isnt present
3)Ensured pending streams are correctly applied when configuration is set
4)Modified discover method to discover streams both with and without configuration.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Outside diff range and nitpick comments (1)
airbyte/sources/base.py (1)

122-122: Line too long.

The line exceeds the maximum length of 100 characters.

-            print("Warning : Configuration is missing. Streams will be selected after configuration is set.")
+            print(
+                "Warning : Configuration is missing. Streams will be selected after configuration is set."
+            )
Tools
Ruff

122-122: Line too long (109 > 100)

(E501)

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 1f80494 and f84437c.

Files selected for processing (1)
  • airbyte/sources/base.py (12 hunks)
Additional context used
Ruff
airbyte/sources/base.py

85-85: Trailing whitespace

Remove trailing whitespace

(W291)


122-122: Line too long (109 > 100)

(E501)


124-124: Blank line contains whitespace

Remove whitespace from blank line

(W293)


174-174: Blank line contains whitespace

Remove whitespace from blank line

(W293)


205-205: Blank line contains whitespace

Remove whitespace from blank line

(W293)


215-215: Too many blank lines (2)

Remove extraneous blank line(s)

(E303)


829-829: No newline at end of file

Add trailing newline

(W292)

Additional comments not posted (11)
airbyte/sources/base.py (11)

79-85: Initialization of new attributes looks good.

The _stream_names_observed and _pending_streams attributes are correctly initialized in the constructor.

Tools
Ruff

85-85: Trailing whitespace

Remove trailing whitespace

(W291)


121-124: Correct handling of missing configuration in select_streams.

The method correctly raises a warning if the configuration is missing and sets _pending_streams.

Tools
Ruff

122-122: Line too long (109 > 100)

(E501)


124-124: Blank line contains whitespace

Remove whitespace from blank line

(W293)


134-140: Logging for missing configuration is appropriate.

The logging message correctly informs the user that the configuration is not set.


174-178: Correct handling of _pending_streams in set_config.

The method correctly applies _pending_streams after setting the configuration.

Tools
Ruff

174-174: Blank line contains whitespace

Remove whitespace from blank line

(W293)


187-188: Appropriate logging for missing configuration in _config property.

The logging message correctly informs the user that the configuration is not set.


200-212: Correct handling of missing configuration and catalog in _discover.

The method correctly handles missing configuration and raises an appropriate error if the catalog is not found.

Tools
Ruff

205-205: Blank line contains whitespace

Remove whitespace from blank line

(W293)


247-249: Correct handling of missing configuration in get_available_streams.

The method correctly logs a warning and returns None if the configuration is missing.


662-674: New method _log_incremental_streams looks good.

The method correctly logs the incremental streams that are currently being used.


675-677: New method _log_stream_read_start looks good.

The method correctly logs the start of stream reading.


788-798: Correct logging of incremental streams in read method.

The method correctly logs incremental streams if known and handles the reading process appropriately.


215-215: Remove extraneous blank line.

There are too many blank lines here.

-        
+

Likely invalid or redundant comment.

Tools
Ruff

215-215: Too many blank lines (2)

Remove extraneous blank line(s)

(E303)

Comment thread airbyte/sources/base.py Outdated
Comment thread airbyte/sources/base.py Outdated
Comment thread airbyte/sources/base.py Outdated
Comment thread airbyte/sources/base.py Outdated
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@aaronsteers

Aaron ("AJ") Steers (aaronsteers) commented Jul 8, 2024

Copy link
Copy Markdown
Member

/test-pr

PR test job started... Check job output.

❌ Tests failed.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between f84437c and 5b43cf1.

Files selected for processing (1)
  • airbyte/sources/base.py (12 hunks)
Additional context used
Ruff
airbyte/sources/base.py

122-122: Line too long (109 > 100)

(E501)


212-212: Too many blank lines (2)

(E303)


827-827: SyntaxError: Expected an expression


827-827: No newline at end of file

(W292)

Additional comments not posted (11)
airbyte/sources/base.py (11)

5-5: Import logging added.

The addition of the logging import is appropriate for the new logging functionality.


79-85: New attributes _stream_names_observed and _pending_streams added.

The new attributes are correctly initialized and will help in managing observed and pending streams.


134-139: Logging for missing configuration.

The logging message for missing configuration is clear and appropriate.


173-176: Handle pending streams in set_config method.

The logic to handle pending streams in the set_config method is correct and ensures streams are selected after configuration is set.


185-186: Logging for missing configuration in _config property.

The logging message for missing configuration is clear and appropriate.


198-209: Logging for missing configuration and catalog in _discover method.

The logging messages for missing configuration and catalog are clear and appropriate.


242-246: Return type modification and logging in get_available_streams method.

The return type modification to list[str] | None and the logging message for missing configuration are appropriate.


249-256: New method _get_incremental_stream_names added.

The method is correctly implemented and retrieves stream names that support incremental sync.


659-671: New method _log_incremental_streams added.

The method is correctly implemented and the logging message for streams using incremental sync mode is clear and appropriate.


672-674: New method _log_stream_read_start added.

The method is correctly implemented and the logging message for the start of stream reading is clear and appropriate.


785-795: Handle incremental streams in read method.

The logic to handle incremental streams and the logging messages are appropriate and correctly implemented.

Comment thread airbyte/sources/base.py Outdated
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 5b43cf1 and 8ab98f7.

Files selected for processing (1)
  • airbyte/sources/base.py (12 hunks)
Additional context used
Ruff
airbyte/sources/base.py

122-122: Line too long (109 > 100)

(E501)


212-212: Too many blank lines (2)

(E303)


827-827: SyntaxError: Expected an expression


827-827: No newline at end of file

(W292)

Additional comments not posted (11)
airbyte/sources/base.py (11)

5-5: Import logging module.

The addition of the logging module is appropriate given the new logging functionality.


79-85: Initialize new class attributes.

The new attributes _stream_names_observed and _pending_streams are correctly initialized.


121-123: Add logging for missing configuration.

The logging statement replaces the print statement, which is more appropriate for warning messages.

Tools
Ruff

122-122: Line too long (109 > 100)

(E501)


133-139: Add logging for missing configuration.

The logging statement is correctly added to handle the case where configuration is not set.


173-176: Apply pending streams after setting configuration.

The logic to apply pending streams after setting configuration is correctly implemented.


185-186: Add logging for missing configuration.

The logging statement is correctly added to handle the case where configuration is not set.


198-209: Add logging for missing configuration or catalog.

The logging statements are correctly added to handle cases where configuration or catalog is missing.


242-246: Add logging for missing configuration.

The logging statement is correctly added to handle the case where configuration is not set.


244-244: Resolve missing method issue.

Ensure requires_config method is declared in the code.


659-671: New method to log incremental streams.

The method correctly logs the streams that are using incremental sync mode.


672-674: New method to log the start of stream read.

The method correctly logs the start of reading a stream.

@aaronsteers

Aaron ("AJ") Steers (aaronsteers) commented Jul 9, 2024

Copy link
Copy Markdown
Member

/fix-pr

Auto-Fix Job Info

This job attempts to auto-fix any linting or formating issues. If any fixes are made,
those changes will be automatically committed and pushed back to the PR.
(This job requires that the PR author has "Allow edits from maintainers" enabled.)

PR auto-fix job started... Check job output.

✅ Changes applied successfully.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 8ab98f7 and db957f8.

Files selected for processing (1)
  • airbyte/sources/base.py (12 hunks)
Additional context used
Ruff
airbyte/sources/base.py

122-122: Line too long (109 > 100)

(E501)

Additional comments not posted (10)
airbyte/sources/base.py (10)

5-5: Import statement for logging looks good.

The addition of the logging module is appropriate for handling the new logging requirements.


79-85: New attributes for managing streams look good.

The additions of _stream_names_observed and _pending_streams are appropriate for managing observed and pending streams.


121-123: Add logging for missing configuration looks good.

The addition of logging for missing configuration is appropriate. However, consider splitting long lines for readability.

- logging.warning("Configuration is missing. Streams will be selected after configuration is set.")
+ logging.warning(
+     "Configuration is missing. Streams will be selected after configuration is set."
+ )
Tools
Ruff

122-122: Line too long (109 > 100)

(E501)


133-139: Add logging for missing available streams looks good.

The addition of logging for when available streams are None is appropriate. However, consider splitting long lines for readability.

- logging.warning("Configuration is not set. Please set the configuration before selecting streams.")
+ logging.warning(
+     "Configuration is not set. Please set the configuration before selecting streams."
+ )

173-176: Logic to handle pending streams looks good.

The addition of logic to handle pending streams in the set_config method is appropriate.


185-186: Add logging for missing configuration looks good.

The addition of logging for missing configuration in the _config property is appropriate.


198-209: Logic and logging for missing configuration and catalog look good.

The addition of logic and logging for missing configuration and catalog in the _discover method is appropriate.


243-245: Add logging for missing configuration looks good but verify the existence of requires_config.

The addition of logging for missing configuration in the get_available_streams method is appropriate. However, ensure the requires_config method is defined.


658-670: New method for logging incremental streams looks good.

The addition of the _log_incremental_streams method to log streams using incremental sync mode is appropriate.


671-673: New method for logging the start of stream reading looks good.

The addition of the _log_stream_read_start method to log when reading starts for a stream is appropriate.

@aaronsteers

Aaron ("AJ") Steers (aaronsteers) commented Jul 9, 2024

Copy link
Copy Markdown
Member

Suraj (@Suraj-Vishwakarma70) - In order to perform a final review on this, we'll need these two CI tests to be in passing (🟢 green) status:

  • Run Tests / Pytest (Fast)
  • Run Tests / Pytest (No Creds)

@Suraj-Vishwakarma70

Suraj (Suraj-Vishwakarma70) commented Jul 9, 2024

Copy link
Copy Markdown
Contributor Author

Aaron ("AJ") Steers (@aaronsteers)
There was some problem in my previous approach . I have fixed it now!
You can have a look :)

@aaronsteers

Aaron ("AJ") Steers (aaronsteers) commented Jul 9, 2024

Copy link
Copy Markdown
Member

/test-pr

PR test job started... Check job output.

❌ Tests failed.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range and nitpick comments (3)
airbyte/sources/base.py (3)

103-103: Remove whitespace from blank line.

There is unnecessary whitespace on this blank line.

-        
+
Tools
Ruff

103-103: Blank line contains whitespace

Remove whitespace from blank line

(W293)


104-109: Use logging instead of print for warnings.

Replace print statements with logging for consistency.

- print("Warning: Config is not set yet. All streams will be selected after config is set.")
+ logging.warning("Config is not set yet. All streams will be selected after config is set.")
- print(f"Warning: Config is not set yet. {streams} streams will be selected after config is set.")
+ logging.warning(f"Config is not set yet. {streams} streams will be selected after config is set.")
Tools
Ruff

107-107: Line too long (102 > 100)

(E501)


109-109: Line too long (109 > 100)

(E501)


824-824: Add trailing newline.

There is no newline at the end of the file.

-]
+]
+
Tools
Ruff

824-824: No newline at end of file

Add trailing newline

(W292)

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 000017a and 5d5a00d.

Files selected for processing (1)
  • airbyte/sources/base.py (5 hunks)
Additional context used
Ruff
airbyte/sources/base.py

103-103: Blank line contains whitespace

Remove whitespace from blank line

(W293)


107-107: Line too long (102 > 100)

(E501)


109-109: Line too long (109 > 100)

(E501)


824-824: No newline at end of file

Add trailing newline

(W292)

Additional comments not posted (4)
airbyte/sources/base.py (4)

84-84: LGTM!

The new attribute _to_be_selected_stream is introduced to store streams that are to be selected later. This is consistent with the objective of handling configurations that are not set initially.


117-121: LGTM!

The modification to handle pending streams and log warnings if the configuration is not set is consistent with the PR objectives.


132-136: LGTM!

The modification to handle pending streams and log warnings if the configuration is not set is consistent with the PR objectives.


180-183: LGTM!

The modification to handle pending streams ensures that streams are selected once the configuration is set.

@aaronsteers

Aaron ("AJ") Steers (aaronsteers) commented Jul 9, 2024

Copy link
Copy Markdown
Member

/fix-pr

Auto-Fix Job Info

This job attempts to auto-fix any linting or formating issues. If any fixes are made,
those changes will be automatically committed and pushed back to the PR.
(This job requires that the PR author has "Allow edits from maintainers" enabled.)

PR auto-fix job started... Check job output.

✅ Changes applied successfully.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Outside diff range and nitpick comments (1)
airbyte/sources/base.py (1)

Line range hint 459-459:
Ensure consistent naming for _to_be_selected_stream.

The attribute _to_be_selected_stream is inconsistently named as _to_be_selected_streams in the read method. Consider standardizing the name for clarity.

- self._to_be_selected_streams = []
+ self._to_be_selected_stream = []
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 5d5a00d and 4486085.

Files selected for processing (1)
  • airbyte/sources/base.py (4 hunks)
Additional context used
Ruff
airbyte/sources/base.py

112-112: Line too long (106 > 100)

(E501)

Additional comments not posted (1)
airbyte/sources/base.py (1)

121-124: Use logging instead of print for warnings.

Replace print statements with logging for consistency.

- self._log_warning_preselected_stream(self._to_be_selected_streams)
+ logging.warning("Config is not set yet. All streams will be selected after config is set.")

Likely invalid or redundant comment.

Comment thread airbyte/sources/base.py Outdated
Comment thread airbyte/sources/base.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 4486085 and 1aa203c.

Files selected for processing (1)
  • airbyte/sources/base.py (4 hunks)
Additional context used
Ruff
airbyte/sources/base.py

112-112: Line too long (106 > 100)

(E501)

Additional comments not posted (3)
airbyte/sources/base.py (3)

121-124: Use logging instead of print for warnings.

Replace print statements with logging for consistency.

- self._log_warning_preselected_stream(self._to_be_selected_streams)
+ logging.warning("Config is not set yet. All streams will be selected after config is set.")

136-139: Use logging instead of print for warnings.

Replace print statements with logging for consistency.

- self._log_warning_preselected_stream(streams)
+ logging.warning(f"Config is not set yet. {streams} streams will be selected after config is set.")

184-187: Ensure consistent naming for _to_be_selected_stream.

The attribute _to_be_selected_stream is inconsistently named as _to_be_selected_streams in the set_config method. Consider standardizing the name for clarity.

- self._to_be_selected_streams = []
+ self._to_be_selected_stream = []

Comment thread airbyte/sources/base.py
Comment thread airbyte/sources/base.py Outdated
@Suraj-Vishwakarma70

Suraj (Suraj-Vishwakarma70) commented Jul 9, 2024

Copy link
Copy Markdown
Contributor Author

/test-pr

@Suraj-Vishwakarma70

Copy link
Copy Markdown
Contributor Author

/test-pr

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 1aa203c and 4e2cc22.

Files selected for processing (1)
  • airbyte/sources/base.py (4 hunks)
Additional context used
Learnings (1)
airbyte/sources/base.py (5)
Learnt from: Suraj-Vishwakarma70
PR: airbytehq/PyAirbyte#285
File: airbyte/sources/base.py:0-0
Timestamp: 2024-07-09T06:37:48.088Z
Learning: In the PyAirbyte project, print statements are preferred over logging for consistency.
Learnt from: Suraj-Vishwakarma70
PR: airbytehq/PyAirbyte#285
File: airbyte/sources/base.py:0-0
Timestamp: 2024-07-09T06:38:40.513Z
Learning: In the PyAirbyte project, print statements are preferred over logging for consistency.
Learnt from: Suraj-Vishwakarma70
PR: airbytehq/PyAirbyte#285
File: airbyte/sources/base.py:104-113
Timestamp: 2024-07-09T06:38:54.843Z
Learning: In the PyAirbyte project, print statements are preferred over logging for consistency.
Learnt from: Suraj-Vishwakarma70
PR: airbytehq/PyAirbyte#285
File: airbyte/sources/base.py:0-0
Timestamp: 2024-07-09T06:37:29.133Z
Learning: In the PyAirbyte project, print statements are preferred over logging for consistency.
Learnt from: Suraj-Vishwakarma70
PR: airbytehq/PyAirbyte#285
File: airbyte/sources/base.py:0-0
Timestamp: 2024-07-09T06:42:41.304Z
Learning: Ensure consistent naming for attributes in the `Source` class in `airbyte/sources/base.py`, such as renaming `_to_be_selected_stream` to `_to_be_selected_streams`.
Ruff
airbyte/sources/base.py

112-112: Line too long (106 > 100)

(E501)

Additional comments not posted (5)
airbyte/sources/base.py (5)

84-84: LGTM!

The initialization of _to_be_selected_streams is correctly handled.


104-113: LGTM!

The use of print statements for logging warnings is consistent with the project's conventions.

Tools
Ruff

112-112: Line too long (106 > 100)

(E501)


121-124: LGTM!

The logic for handling missing configuration and logging a warning is correct.


136-139: LGTM!

The logic for handling missing configuration and logging a warning is correct.


184-187: LGTM!

The logic for handling _to_be_selected_streams and selecting streams is correct.

@Suraj-Vishwakarma70

Copy link
Copy Markdown
Contributor Author

/fix-pr

Comment thread airbyte/sources/base.py
@aaronsteers

Aaron ("AJ") Steers (aaronsteers) commented Jul 9, 2024

Copy link
Copy Markdown
Member

/test-pr

PR test job started... Check job output.

❌ Tests failed.

@aaronsteers

Aaron ("AJ") Steers (aaronsteers) commented Jul 9, 2024

Copy link
Copy Markdown
Member

Suraj (@Suraj-Vishwakarma70) - I think this is close. Lint checks are passing now and the fast & no-creds tests are passing. If full tests pass at this stage, I think we will be ready to merge.

Quick note: unfortunately, /fix-pr and /test-pr will only work for administrators, due to permissions.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 4e2cc22 and c285c94.

Files selected for processing (1)
  • airbyte/sources/base.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
  • airbyte/sources/base.py

@aaronsteers Aaron ("AJ") Steers (aaronsteers) changed the title Fix: Raise a warning if config is not present Fix: Instead of failing, raise a warning if stream selection is performed before config is present Jul 9, 2024
@aaronsteers Aaron ("AJ") Steers (aaronsteers) merged commit f5f7b03 into airbytehq:main Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🐛 Bug: Unexpected failure when select_streams() is called before set_config()

2 participants