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

DDST-229: Add spreadsheet alter. #21

Closed
wants to merge 2 commits into from
Closed

DDST-229: Add spreadsheet alter. #21

wants to merge 2 commits into from

Conversation

chrismacdonaldw
Copy link
Contributor

No description provided.

@chrismacdonaldw chrismacdonaldw added the patch Backwards compatible bug fixes. label Jul 15, 2024
];

foreach ($fields_to_add as $field) {
$process[$field[0]][2]['values'][$field[1]] = [
Copy link
Contributor

Choose a reason for hiding this comment

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

A little spooked by the direct/blind overwrite here. Assertions of some sort might be in order, to make sure that what we're stomping on is what is expect, in case the upstream migration changes in some manner.

Comment on lines +29 to +52
// Add Text (plain) delimited fields.
$fields_to_add = [
['field_local_contexts', 'local_contexts', ';'],
['field_access_id', 'access_id', ';'],
];

foreach ($fields_to_add as $field) {
$process[$field[0]] = [
[
'plugin' => 'skip_on_empty',
'source' => $field[1],
'method' => 'process',
],
[
'plugin' => 'dgi_migrate.process.explode',
'delimiter' => $field[2],
],
[
'plugin' => 'skip_on_empty',
'method' => 'row',
'message' => 'Empty ' . $field[1] . '.',
],
];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Better structure of data could make things much more legible:

Suggested change
// Add Text (plain) delimited fields.
$fields_to_add = [
['field_local_contexts', 'local_contexts', ';'],
['field_access_id', 'access_id', ';'],
];
foreach ($fields_to_add as $field) {
$process[$field[0]] = [
[
'plugin' => 'skip_on_empty',
'source' => $field[1],
'method' => 'process',
],
[
'plugin' => 'dgi_migrate.process.explode',
'delimiter' => $field[2],
],
[
'plugin' => 'skip_on_empty',
'method' => 'row',
'message' => 'Empty ' . $field[1] . '.',
],
];
}
// Add Text (plain) delimited fields.
$fields_to_add = [
'field_local_contexts' => ['local_contexts', ';'],
'field_access_id' => ['access_id', ';'],
];
foreach ($fields_to_add as $field => $info) {
[$source, $delimiter] = $info;
$process[$field] = [
[
'plugin' => 'skip_on_empty',
'source' => $source,
'method' => 'process',
],
[
'plugin' => 'dgi_migrate.process.explode',
'delimiter' => $delimiter,
],
[
'plugin' => 'skip_on_empty',
'method' => 'row',
'message' => 'Empty ' . $source . '.',
],
];
}

[
'plugin' => 'skip_on_empty',
'method' => 'row',
'message' => 'Empty ' . $field[1] . '.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure about the specific message here: The issue is not that it's empty, per-se, as we're fine when it's empty, skipping the processing up front, but that the source value contained only delimiters (and maybe whitespace that was trimmed?).

Maybe something like:

Suggested change
'message' => 'Empty ' . $field[1] . '.',
'message' => "No usable values in {$field[1]}.",

(but would change if adopting the more specific variable names, whatever)

}

// Add Text (plain) non-delimited fields.
$fields_to_add = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure where to comment on exactly, but I'm thinking we should generally avoid the variable reuse across these bigger functions.

More specific names, with more specific structures should help future readability.

$process[$field[0]][] = [
'plugin' => 'skip_on_empty',
'method' => 'row',
'message' => 'Empty ' . $field[0] . '.',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we're kind of in the same "no useful value" case here?

[
'plugin' => 'skip_on_empty',
'method' => 'row',
'message' => 'Empty ' . $field[2] . '.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Same kind of "no useful value" case here?

@dynac01 dynac01 closed this Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Backwards compatible bug fixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants