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

Export code clean up - extract build row & getTransformed row off to ExportProcessor #13117

Merged
merged 4 commits into from
Nov 18, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Code cleanup in export class

Before

Code not extracted

After

Code extracted

Technical Details

@mattwire this is actually a pretty simple code moving PR - it should be fairly easy to verify by repeating the steps to extract locally. I think the jenkins tests should be pretty sufficient here as there is no logical change in this step

Comments

@civibot
Copy link

civibot bot commented Nov 16, 2018

(Standard links)

@civibot civibot bot added the master label Nov 16, 2018
@eileenmcnaughton
Copy link
Contributor Author

@mattwire I also added this - #13120 which doesn't conflict with this so can be tested in conjunction

@eileenmcnaughton
Copy link
Contributor Author

@mattwire
Copy link
Contributor

I've tested this locally with a small dataset (207 contacts) and it works fine with/without this PR. The PR itself is extracting and moving code to the ExportProcessor class and creating a new function to build each row. There is a small risk that this could have a cumulative performance impact on large datasets but given that the functions called multiple times are cached (pseudoconstant and i18n singleton) that impact should be minimal. Ok to merge @eileenmcnaughton @seamuslee001

@eileenmcnaughton eileenmcnaughton merged commit 0df8440 into civicrm:master Nov 18, 2018
@eileenmcnaughton eileenmcnaughton deleted the export2 branch November 18, 2018 21:57
@eileenmcnaughton
Copy link
Contributor Author

thanks @mattwire I'll push up another step later today

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.

2 participants