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

CDK: Refactor JSONSchemaResolver and ResourceSchemaLoader for better testing #3222

Closed
sherifnada opened this issue May 4, 2021 · 0 comments · Fixed by #6044
Closed

CDK: Refactor JSONSchemaResolver and ResourceSchemaLoader for better testing #3222

sherifnada opened this issue May 4, 2021 · 0 comments · Fixed by #6044
Assignees
Labels
accepting-contributions Feel free to contribute to them CDK Connector Development Kit good first issue Good for newcomers type/enhancement New feature or request

Comments

@sherifnada
Copy link
Contributor

sherifnada commented May 4, 2021

Tell us about the problem you're trying to solve

There is a number of improvements that can be made to classes in airbyte_cdk/schema_helpers.py.

First, JsonSchemaResolver can be refactored to have the following signature:

def __init__(schemas: Mapping[str, any], refs: Mapping[str, any])
  # schemas are all json files in `schemas/`, refs are all json files in `schemas/shared/`Then we can thoroughly test the resolving behavior in `JsonSchemaResolver` independently from the logic for where we actually load schemas from. 

Then ResourceSchemaLoader would be responsible just for finding the right files in the package directory and passing them into JsonSchemaResolver. We can even change the signature for RSL to be:

class ResourceSchemaLoader:
def ___init__(package_name: str, json_schema_resolver: JsonSchemaResolver): 

this would allow us to mock JsonSchemaResolver and instead just validate that given the right file structure, RSL is passing the correct files to JSR.

┆Issue is synchronized with this Asana task by Unito

@sherifnada sherifnada added type/enhancement New feature or request good first issue Good for newcomers accepting-contributions Feel free to contribute to them CDK Connector Development Kit labels May 4, 2021
davinchia pushed a commit that referenced this issue May 5, 2021
closes #2722

I don't love the current structure. There is a number of improvements that can be made:

JsonSchemaResolver can be refactored to have the following signature:

def __init__(schemas: Mapping[str, any], refs: Mapping[str, any])
  # schemas are all json files in `schemas/`, refs are all json files in `schemas/shared/`
Then we can thoroughly test the resolving behavior in JsonSchemaResolver independently from the logic for where we actually load schemas from.

I was tempted to make this refactor now but I'd rather cover more ground with tests then revisit. I created an issue to track this: #3222
@avida avida self-assigned this Sep 14, 2021
@avida avida added this to the Connectors, September 17th 2021 milestone Sep 14, 2021
@avida avida linked a pull request Sep 14, 2021 that will close this issue
38 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepting-contributions Feel free to contribute to them CDK Connector Development Kit good first issue Good for newcomers type/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants