-
Notifications
You must be signed in to change notification settings - Fork 74
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
refactor(targets): Added a class method to instantiate JSONSchemaToSQL
from the target configuration
#2782
refactor(targets): Added a class method to instantiate JSONSchemaToSQL
from the target configuration
#2782
Conversation
CodSpeed Performance ReportMerging #2782 will not alter performanceComparing Summary
|
bc026c7
to
934ef21
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2782 +/- ##
==========================================
- Coverage 91.37% 91.33% -0.04%
==========================================
Files 62 62
Lines 5203 5207 +4
Branches 675 675
==========================================
+ Hits 4754 4756 +2
- Misses 317 319 +2
Partials 132 132 ☔ View full report in Codecov by Sentry. |
55b3d2c
to
4be6949
Compare
JSONSchemaToSQL
from the tap configurationJSONSchemaToSQL
from the target configuration
4be6949
to
de53f85
Compare
…the tap configuration
de53f85
to
42a702d
Compare
Reviewer's Guide by SourceryThis pull request introduces a Updated class diagram for JSONSchemaToSQLclassDiagram
class JSONSchemaToSQL {
- _fallback_type: type[sa.types.TypeEngine]
+ __init__(max_varchar_length: int | None = None) : None
+ from_config(config: dict, max_varchar_length: int | None) : JSONSchemaToSQL
+ _invoke_handler(handler: JSONtoSQLHandler, json_type: str, jsonschema: dict, column_name: str) : sa.types.TypeEngine
+ register_column_name_handler(json_type: str, handler: ColumnNameHandler) : None
+ register_type_handler(json_type: str, handler: JSONtoSQLHandler) : None
+ register_format_handler(json_format: str, handler: JSONtoSQLHandler) : None
+ column_name_mapping(column_name: str) : str
+ convert_column(column_name: str, jsonschema: dict) : sa.Column
}
Updated class diagram for SQLConnectorclassDiagram
class SQLConnector {
- config: dict | None
- authenticator: AuthenticatorBase | None
- state: dict
- _metadata_cache: dict
+ __init__(config: dict | None = None, parse_env_config: bool = False, state: dict | None = None)
+ discover() : dict
+ _discover_catalog() : dict
+ get_catalog() : dict
+ discover_streams() : list[dict]
+ sync(*args, **kwargs) : None
+ _sync_stream(stream: SingerStream, catalog: dict) : None
+ get_records(context: dict) : t.Iterable[dict]
+ get_schema(context: dict) : dict
+ get_singer_stream(stream_name: str, catalog: dict) : SingerStream
+ _get_row_count(table_name: str) : int
+ open_connection() : sa.engine.Connection
+ bulk_insert_records(table_name: str, records: t.Iterable[dict], stream: SingerStream) : None
+ create_empty_table(stream: SingerStream, schema: dict) : None
+ jsonschema_to_sql() : JSONSchemaToSQL
+ _connect() : t.Iterator[sa.engine.Connection]
+ to_sql_type(jsonschema_type: str, jsonschema_format: str | None = None) : sa.types.TypeEngine
+ get_table(table_name: str) : sa.Table
+ prepare_schema(schema: dict) : dict
+ get_starting_replication_key_value(context: dict) : t.Any
+ get_replication_key_value(record: dict, context: dict) : t.Any
+ finalize_state_message(state_message: dict) : dict
+ write_bookmark(state: dict, stream_name: str, state_value: t.Any) : None
+ write_state_message(state: dict) : None
+ close() : None
+ ping_connection() : bool
+ validate_config(raise_errors: bool = True) : bool
+ apply_catalog(catalog: dict) : None
+ get_fully_qualified_name(table_name: str) : str
+ jsonschema_to_sql_converter: type[JSONSchemaToSQL]
+ sql_to_jsonschema_converter: type[SQLToJSONSchema]
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @edgarrmondragon - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a test case that demonstrates the usage of
from_config
with a custom configuration. - It might be helpful to include a note about the expected structure of the
config
dictionary in thefrom_config
docstring.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -574,7 +608,10 @@ def jsonschema_to_sql(self) -> JSONSchemaToSQL: | |||
|
|||
.. versionadded:: 0.42.0 | |||
""" | |||
return JSONSchemaToSQL(max_varchar_length=self.max_varchar_length) |
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.
issue (complexity): Consider simplifying the code by directly instantiating the converter in the cached property and passing the config to the constructor, instead of using a from_config
method.
The extra layer of abstraction may not be worth the complexity unless you plan to extend the configuration logic in the near future. If you want to simplify while keeping future customization in mind, consider directly instantiating the converter in the cached property and use the class attribute for customization when needed.
For example, instead of:
@functools.cached_property
def jsonschema_to_sql(self) -> JSONSchemaToSQL:
return self.jsonschema_to_sql_converter.from_config(
self.config,
max_varchar_length=self.max_varchar_length,
)
you might simplify it to:
@functools.cached_property
def jsonschema_to_sql(self) -> JSONSchemaToSQL:
return self.jsonschema_to_sql_converter(
max_varchar_length=self.max_varchar_length,
config=self.config
)
Additionally, update your converter's constructor to accept a config
parameter if needed. This streamlines the instantiation while retaining the ability to extend behavior via subclassing.
📚 Documentation preview 📚: https://meltano-sdk--2782.org.readthedocs.build/en/2782/
Summary by Sourcery
This pull request introduces a
from_config
class method to theJSONSchemaToSQL
class, allowing instantiation of the class from the target configuration. It also adds ajsonschema_to_sql_converter
property toSQLConnector
to allow overriding the defaultJSONSchemaToSQL
class. This change allows users to customize theJSONSchemaToSQL
class using the target configuration.Enhancements:
from_config
onJSONSchemaToSQL
to allow instantiation of the class from the target configuration.jsonschema_to_sql_converter
property toSQLConnector
to allow overriding the defaultJSONSchemaToSQL
class.Documentation:
JSONSchemaToSQL
class.