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] Cleanup custom field handling... twice #16989

Merged
merged 1 commit into from
Apr 6, 2020

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Apr 5, 2020

Overview

What should have been a minor code cleanup, but in two places ☹️

Technical Details

Yes, this is exactly the same code in two places.

Comments

Part of me died inside when I saw this.

@civibot
Copy link

civibot bot commented Apr 5, 2020

(Standard links)

@civibot civibot bot added the master label Apr 5, 2020
@colemanw colemanw changed the title Cleanup custom field handling... twice [REF] Cleanup custom field handling... twice Apr 5, 2020
@pradpnayak
Copy link
Contributor

pradpnayak commented Apr 6, 2020

Looks good to me but do we need to move the duplicate code into separate function? Anyways its good to merge!

@colemanw
Copy link
Member Author

colemanw commented Apr 6, 2020

Yes, there is a lot of cleanup to do. It would be simple enough to consolidate these two chunks of copy/paste, but, one of them is in DeprecatedUtils so already needs to be removed. And the other is in an Import class. There are 5 of those, and they all basically copy/paste each other, so what's really needed is to move custom data parsing out of the 5 separate CRM_*_Import_Parser classes and into their shared parent CRM_Import_Parser.
If you'd like to help with any of that, yes please!

@colemanw colemanw merged commit 834f0bd into civicrm:master Apr 6, 2020
@colemanw colemanw deleted the seeingDouble branch September 12, 2020 02:50
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