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

refactor: Remove unused connector trait method #830

Closed

Conversation

karolisg
Copy link
Contributor

@karolisg karolisg commented Feb 8, 2023

No description provided.

@karolisg karolisg marked this pull request as ready for review February 8, 2023 09:18
@coveralls
Copy link

Pull Request Test Coverage Report for Build 4122569427

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 105 unchanged lines in 8 files lost coverage.
  • Overall coverage decreased (-0.3%) to 67.558%

Files with Coverage Reduction New Missed Lines %
dozer-api/src/grpc/typed/tests/service.rs 2 97.91%
dozer-core/src/executor.rs 6 86.61%
dozer-ingestion/src/connectors/events/connector.rs 8 34.48%
dozer-ingestion/src/connectors/mod.rs 9 14.29%
dozer-ingestion/src/connectors/kafka/connector.rs 14 0%
dozer-ingestion/src/connectors/postgres/connector.rs 20 0%
dozer-ingestion/src/connectors/ethereum/connector.rs 22 0%
dozer-ingestion/src/connectors/snowflake/connector.rs 24 0%
Totals Coverage Status
Change from base Build 4121316108: -0.3%
Covered Lines: 23107
Relevant Lines: 34203

💛 - Coveralls

@v3g42
Copy link
Contributor

v3g42 commented Feb 9, 2023

@karolisg On second thought, shoudl we remove get_schemas and leave get_tables instead ? Basically whicher has information sufficient for both cases.

@karolisg
Copy link
Contributor Author

karolisg commented Feb 9, 2023

@karolisg On second thought, shoudl we remove get_schemas and leave get_tables instead ? Basically whicher has information sufficient for both cases.

@v3g42 get_tablesis supposed to return all tables and columns, while get_schemas is only used to map selected tables
with only selected columns to our schema. It might not be very efficient to fetch all tables and map them to schemas on every app run/migration.

@v3g42
Copy link
Contributor

v3g42 commented Feb 9, 2023

I know the current way it is implemented doesn't make sense.

What I want to know typically is

  • Names of the tables
  • Field Definitions
  • Indexes defined on these tables.

Querying all vs select tables should be a property on the method and it is a user's choice. For example, I want to first view all the available tables and definitions before I know what tables to use.

 fn get_tables(&self) -> Result<Vec<TableInfo>, ConnectorError> {
        self.schema_helper.get_tables(None)
    }

    fn get_schemas(
        &self,
        table_names: Option<Vec<TableInfo>>,
    ) -> Result<Vec<SchemaWithChangesType>, ConnectorError> {
        self.schema_helper
            .get_schemas(table_names)
            .map_err(ConnectorError::PostgresConnectorError)
    }

@karolisg karolisg force-pushed the refactor/remove-unused-methods branch from e615fc7 to c725eec Compare February 13, 2023 08:42
@karolisg karolisg closed this Feb 13, 2023
@karolisg karolisg force-pushed the refactor/remove-unused-methods branch from c725eec to f082dc2 Compare February 13, 2023 08:53
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.

None yet

4 participants