Skip to content
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: Remove extra '--' #20

Closed
wants to merge 6 commits into from
Closed

fix: Remove extra '--' #20

wants to merge 6 commits into from

Conversation

SpaceCondor
Copy link
Contributor

Remove bug introduced in latest commit. The docker containers do not expect the arguments with --. This is noted here:

e1349aa#commitcomment-143275245

@SpaceCondor SpaceCondor changed the title Remove extra '--' fix: Remove extra '--' Jul 25, 2024
@edgarrmondragon edgarrmondragon requested a review from z3z1ma July 25, 2024 15:49
Copy link
Member

Choose a reason for hiding this comment

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

Why remove these files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to see if just the test_core.py would run successfully. I was planning on adding them back after I could get the first set of tests passing.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can restore these now. Wdyt?

Copy link
Contributor Author

@SpaceCondor SpaceCondor Jul 30, 2024

Choose a reason for hiding this comment

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

@edgarrmondragon I restored them, but many were out of date and no longer available on pypi. I updated them, but I dropped the test_pub_apis_sync since the Meltano SDK was throwing an error about AirbyteStream not having a schema property.

It looks like the singer_sdk has the following check in the Stream class:

    if schema:
      ....
       elif isinstance(schema, dict):
            self._schema = schema
        ......
    if not self.schema:
        ......

But when an empty dict is passed (which happens in the test) it throws an error.

..\..\..\..\..\AppData\Local\pypoetry\Cache\virtualenvs\tap-airbyte-nuA4U9RQ-py3.10\lib\site-packages\singer_sdk\streams\core.py:171: in __init__
    if not self.schema:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <tap_airbyte.tap.AirbyteStream object at 0x000001A0BFE97EE0>

    @property
    def schema(self) -> dict:
        """Get schema.
    
        Returns:
            JSON Schema dictionary for this stream.
        """
>       return self._schema
E       AttributeError: 'AirbyteStream' object has no attribute '_schema'. Did you mean: 'schema'?
..\..\..\..\..\AppData\Local\pypoetry\Cache\virtualenvs\tap-airbyte-nuA4U9RQ-py3.10\lib\site-packages\singer_sdk\streams\core.py:459: AttributeError

@edgarrmondragon
Copy link
Member

@SpaceCondor this is looking good! Wdyt of moving d6f028b to a separate PR so it's easier to call out in release notes for this tap?

@SpaceCondor
Copy link
Contributor Author

@edgarrmondragon Sure! I can do that now. Although the tests for 3.8 will fail 😅

@edgarrmondragon
Copy link
Member

@edgarrmondragon Sure! I can do that now. Although the tests for 3.8 will fail 😅

Oh we can definitely drop support for 3.8 in that PR :)

@edgarrmondragon edgarrmondragon self-assigned this Aug 7, 2024
@SpaceCondor SpaceCondor closed this by deleting the head repository Aug 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants