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.

Sign up for GitHub

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

Jump to bottom

Add type casting to CSV fields #82

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Add type casting to CSV fields #82

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f5220bb
Add type casting to CSV fields
Libertie Oct 7, 2021
6125efa
Correct fields prop default
Libertie Oct 8, 2021
524680c
Refactor to typeCast method + add error handling
Libertie Oct 12, 2021
4bc1a68
Document new invalidFieldType text
Libertie Oct 12, 2021
ba37f17
Additional error handling for typeCast method
Libertie Oct 15, 2021
7cf7ae2
Expand documentation for type casting
Libertie Oct 15, 2021
d628eff
Reorganize to eliminate redundancy in typeCast
Libertie Oct 15, 2021
2e728c8
Filter csv map to prevent typeCast errors
Libertie Oct 17, 2021
0c21a32
Reduce nesting in buildMappedCsv()
Libertie Oct 22, 2021
dfbc006
Bump cli-plugin-unit-jest version
Libertie Feb 15, 2022
e045a72
Update for republish on npmjs.com
Libertie Apr 8, 2023
ceb547b
Merge remote-tracking branch 'upstream/master'
Libertie Apr 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ Primary wrapper component.
},
age: {
required: true,
label: 'Age'
label: 'Age',
type: Number
}
}"
>
Expand All @@ -101,11 +102,21 @@ Primary wrapper component.

| Prop | Default | Description |
| ------ | ------- | ----------- |
| fields | null | (required) The field names used to map the CSV. |
| fields | see below | (required) The fields used to map the CSV. |
Libertie marked this conversation as resolved.
Show resolved Hide resolved
| text | see below | (optional) Override the default text used in the component. |
| modelValue | N/A | (optional) Binds to the mapped CSV object. |

#### Default text
##### Fields Prop

The fields prop may be a simple array (e.g. `['name', 'age']) or an object with the following properties:

| Prop | Default | Description |
| ------ | ------- | ----------- |
| required | true | (required) The field names used to map the CSV. |
| label | N/A | (required) Override the default text used in the component. |
| type | String | (optional) A primitive object used to cast the field value. |

##### Default Text Prop

```json
{
Expand Down
4 changes: 3 additions & 1 deletion src/components/VueCsvImport.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
key: key,
label: get(val, 'label', val),
required: get(val, 'required', true),
type: get(val, 'type', String),
};
});
}
Expand Down Expand Up @@ -83,11 +84,12 @@

const buildMappedCsv = function () {
let newCsv = VueCsvImportData.fileHasHeaders ? VueCsvImportData.rawCsv : drop(VueCsvImportData.rawCsv);
let typeMap = VueCsvImportData.fields.reduce((a, f) => set(a, f.key, f.type ?? String), {});
Libertie marked this conversation as resolved.
Show resolved Hide resolved

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)));
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

This is implemented in the newest commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

🔥 looking good! will have a proper look in the AM 👍

});

return newRow;
Expand Down