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

[NFC] Cleanup towards CRM-20759, define variables more legibly & consistently. #10554

Merged
merged 1 commit into from
Jun 27, 2017

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jun 23, 2017

I really want the 2 copy & paste variants of this code loop to be combinable but I think it takes a numebr
of small changes to get there in a readable way

@totten - this is how I was planning on following up on that extraction - turning those weirdly assigned vars into an array & standardising across functions


@@ -782,19 +758,19 @@ public function submit($params, $mapperKeys) {
if (($first == 'a' && $second == 'b') ||
($first == 'b' && $second == 'a')
) {
$relatedVal = $this->_mapperFields[$fldName];
$parserParameters['mapperRelated'][$i] = $this->_mapperFields[$fldName];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this to match the other function's name - not sure it makes any more sense though :-)


//set main contact properties.
$properties = array(
'ims' => 'mapperImProvider',
'mapper' => 'mapper',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this can go too. I just stopped short to try to keep the focus narrow

@eileenmcnaughton eileenmcnaughton force-pushed the crazy_vars branch 2 times, most recently from ae56dd3 to 4d6b0b7 Compare June 23, 2017 23:30
@eileenmcnaughton eileenmcnaughton changed the title Cleanup towards CRM-20759, define variables more legibly & consistently. [NFC] Cleanup towards CRM-20759, define variables more legibly & consistently. Jun 23, 2017
@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton eileenmcnaughton force-pushed the crazy_vars branch 4 times, most recently from ccb3463 to 741ceef Compare June 26, 2017 05:01
I really want the 2 copy & paste variants of this code loop to be combinable but I think it takes a numebr
of small changes to get there in a readable way
@@ -717,39 +717,15 @@ public function formatCustomFieldName(&$fields) {
* @return \CRM_Contact_Import_Parser_Contact
*/
public function submit($params, $mapperKeys) {
$mapper = $mapperKeysMain = array();
$mapper = $mapperKeysMain = $locations = array();
$parserParameters = CRM_Contact_Import_Parser_Contact::getParameterForParser($this->_columnCount);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently this just gets a base populated array - later I would like to move the whole derivation of $parserParameters into there & share between the 2 functions -but I feel that is too much for one pr

);

//set respective mapper params to array.
foreach (array_keys($mapperParams) as $mapperParam) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This in conjunction with the $$mapperParam = NULL; loop does the same as CRM_Contact_Import_Parser_Contact::getParameterForParser

}
}

//set the respective mapper param array values.
foreach ($mapperParams as $mapperParamKey => $mapperParamVal) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of this is to transfer the value e.g $relatedContactTypeVal into the $relatedContactType array - weird huh!

}

$this->set('columnNames', $this->_columnNames);

//set main contact properties.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replaced by straightforward set statements below

Copy link
Contributor

@seamuslee001 seamuslee001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to make sense to me @eileenmcnaughton

@eileenmcnaughton
Copy link
Contributor Author

Thanks @seamuslee001 - merging. I'm still working on code over the top of this so it's getting further testing in there

@eileenmcnaughton eileenmcnaughton merged commit 3b32e27 into civicrm:master Jun 27, 2017
@eileenmcnaughton eileenmcnaughton deleted the crazy_vars branch June 27, 2017 01:12
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