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

(REF) Importer - Remove unused parameters. Simplify signature. #23401

Merged
merged 1 commit into from
May 10, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

[Import] Fix database initialization to remove unused parameters

Before

The function that sets up the datasource is called postProcess (a very formy name) and receives 3 parameters, none of which are used anymore -

After

The function is renamed initialize and takes no parameters

Technical Details

We are pretty close to the point where we could usefully bikeshed how the datasource class might look - assuming we imaging csv & sql query might not be the only 2 ever - I don' tthink that should hold this up merging but if there is interest in discussing at this stage I'll open a gitlab. Here are how the current public functions look....

image

& here is which ones are actually overridden

image

Comments

@civibot
Copy link

civibot bot commented May 7, 2022

(Standard links)

@civibot civibot bot added the master label May 7, 2022
@totten totten changed the title [Import] Fix database initialization to remove unused parameters (REF) Importer - Remove unused parameters. Simplify signature. May 9, 2022
@totten
Copy link
Member

totten commented May 9, 2022

Grepped universe and confirmed that CRM_Import_DataSource is core-only, so we're free to refactor signatures in there.

@colemanw colemanw merged commit 3fbdad5 into civicrm:master May 10, 2022
@colemanw colemanw deleted the import_data_source branch May 10, 2022 00:11
@colemanw
Copy link
Member

Refactor looks good.

@eileenmcnaughton
Copy link
Contributor Author

thanks @colemanw

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants