Add type casting to CSV fields #82
Conversation
src/components/VueCsvImport.vue
Outdated
|
|
||
| VueCsvImportData.value = map(newCsv, (row) => { | ||
| let newRow = {}; | ||
| forEach(VueCsvImportData.map, (column, field) => { | ||
| set(newRow, field, get(row, column)); | ||
| set(newRow, field, typeMap[field](get(row, column))); |
There was a problem hiding this comment.
It might be a good idea to throw in some kind of error handing to this?
If I had a string in the field Number('abc') -> NaN
There was a problem hiding this comment.
Sure thing. We can push a message to VueCsvImportData.errors (e.g. "Invalid data type in row X column Y.").
In terms of fallback, we could use the original string value when a field is otherwise invalid. Would it be overkill to offer a strictTypeCasting option for situations when we actually want to convert invalid fields to null?
There was a problem hiding this comment.
Pushing to errors and falling back to the original sounds like a good plan 👍
strict option does sound like overkill. imo, I would offload something like this to the server.
There was a problem hiding this comment.
This is implemented in the newest commit.
There was a problem hiding this comment.
🔥 looking good! will have a proper look in the AM 👍
|
Oh, dang. Just realized that boolean type casting isn't working because |
src/components/VueCsvImport.vue
Outdated
| case 'false': case 'no': case '0': case 'null': case '': return false; | ||
| case 'true': case 'yes': case '1': return true; |
There was a problem hiding this comment.
fieldVal.toLowerCase().trim() === 'false' ? false : !!string Could be an option too but I do like the idea of being able to pass these other values. "on/off" also comes to mind.
There was a problem hiding this comment.
I'll have time to push a documentation update tomorrow and I'll add on/off to the boolean switch statement.
There was a problem hiding this comment.
OK, just pushed a few more commits to handle on/off booleans, improve error handling, and better document the new functionality. Wow. This really turned into a big thing. 😅
|
Did a little more testing and ran into a bug that I need to fix. Basically, if you map a field and then unmap it, an error gets thrown incorrectly. This is because the field remains in the A solution is to simply exclude fields with a column value of null when updating |
src/components/VueCsvImport.vue
Outdated
| let newRow = {}; | ||
| forEach(VueCsvImportData.map, (column, field) => { | ||
| set(newRow, field, get(row, column)); | ||
| forEach(pickBy(VueCsvImportData.map, (v) => Number.isInteger(v)), (column, field) => { |
There was a problem hiding this comment.
This nested logic is pretty hairy.
Am I correct in assuming you are trying to do this?
VueCsvImportData
.filter((v) => Number.isInteger(v))
.map(([column, field]) => { try... })
There was a problem hiding this comment.
No, that's not quite it. Here is that code fully commented, with the bit you highlighted split out for clarity:
VueCsvImportData.value = map(newCsv, (row, index) => {
// Start a new row
let newRow = {};
// We only want to include mapped fields, so we'll exclude anything
// in VueCsvImportData.map that isn't mapped to a column number
let currentlyMapped = pickBy(VueCsvImportData.map, (v) => Number.isInteger(v));
// Loop through the mapped fields and build up a new row with each field
forEach(currentlyMapped, (column, field) => {
// Get the field value
let fieldVal = get(row, column);
// Try casting the field value
try {
fieldVal = typeCast(field, fieldVal);
// If that generates an error, add a message to the VueCsvImportData.errors array
} catch(err) {
VueCsvImportData.errors.push(
defaultLanguage.errors.invalidFieldType
.replace(/\${row}/g, index + (VueCsvImportData.fileHasHeaders ? 1 : 2))
.replace(/\${col}/g, column + 1)
);
// Either way, add it to the row
} finally {
set(newRow, field, fieldVal);
}
});
// Return the completed row!
return newRow;
});Filtering VueCsvImportData.map is necessary because if you selected a column for a field, but then later unselected it, the field remains in the map object but is assigned a null value. In order to loop through the map and build up the new row, we have to exclude these unmapped fields. Otherwise they will generate errors.
I'm happy to add some or all of the commenting above to my code. And, of course, if you seen opportunities to improve it please let me know!
There was a problem hiding this comment.
Ok that makes sense now! Nice catch 🥇
Could you just update the method to moved the nested curreltyMapped into what it looks like in your comment above. ie:
| forEach(pickBy(VueCsvImportData.map, (v) => Number.isInteger(v)), (column, field) => { | |
| // We only want to include mapped fields because deselected fields remain in the map object | |
| // so we'll exclude anything in VueCsvImportData.map that isn't mapped to a column number | |
| const currentlyMapped = pickBy(VueCsvImportData.map, (v) => Number.isInteger(v)); | |
| forEach(currentlyMapped, (column, field) => { |
There was a problem hiding this comment.
Sounds good. And it looks like we can actually get a slight performance boost by moving that variable assignment out of the map function. I'll push a commit.
|
|
Kudos, SonarCloud Quality Gate passed!
|
|
Oops. I was just looking back at this and accidentally closed the request. (I reopened it.) |
|
@jgile, any chance of getting this PR merged soon? |
#81