[low-code] replace emptySchemaLoader with DefaultSchemaLoader #18947
Conversation
| @@ -59,7 +58,6 @@ | |||
| "DefaultErrorHandler": DefaultErrorHandler, | |||
| "DefaultPaginator": DefaultPaginator, | |||
| "DpathExtractor": DpathExtractor, | |||
| "EmptySchemaLoader": EmptySchemaLoader, | |||
There was a problem hiding this comment.
Removed this entirely since I don't really see a use case where a developer should need to define the default.
There was a problem hiding this comment.
is it worth adding the default schema loader?
There was a problem hiding this comment.
That's what I meant that we wouldn't need the default loader defined since we don't expect developers to need to define it themselves.
| from airbyte_cdk.sources.declarative.schema.schema_loader import SchemaLoader | ||
| from airbyte_cdk.sources.declarative.types import Config | ||
| from dataclasses_jsonschema import JsonSchemaMixin | ||
|
|
||
|
|
||
| @dataclass | ||
| class EmptySchemaLoader(SchemaLoader, JsonSchemaMixin): | ||
| class DefaultSchemaLoader(SchemaLoader, JsonSchemaMixin): |
There was a problem hiding this comment.
The alternative here would have been to fold the defaulting to the empty schema logic into the existing JsonFileSchemaLoader and have that remain as the default. The drawback there is that it starts to blur the line between how errors should be reported when schema files don't exist. I think its good to have very clear failures when a developer explicitly defines their SchemaLoader like when a file does not exist. However, when using the default we should just continue w/ the empty schema like we do for this component.
| @@ -59,7 +58,6 @@ | |||
| "DefaultErrorHandler": DefaultErrorHandler, | |||
| "DefaultPaginator": DefaultPaginator, | |||
| "DpathExtractor": DpathExtractor, | |||
| "EmptySchemaLoader": EmptySchemaLoader, | |||
There was a problem hiding this comment.
is it worth adding the default schema loader?
| return {} | ||
| try: | ||
| return self.default_loader.get_json_schema() | ||
| except FileNotFoundError: |
|
/publish-cdk dry-run=false
|
What
In #18532 overlooked that now a lot of our contributors might not have defined a
JsonSchemain their manifest because it was defaulted. Switching with the EmptySchemaLoader is a regression in being able to read schemasHow
Replaces/renames the
EmptySchemaLoaderasDefaultSchemaLoaderwhose default functionality will be to attempt to read from the default location, otherwise return the empty schema.Recommended reading order
default_schema_loader.py