-
Notifications
You must be signed in to change notification settings - Fork 6
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
action to process a new dataTable #303
action to process a new dataTable #303
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @surchs . Looks good. I just made two comments. I'll let you decide if you want to implement the suggested change in the first.
@@ -100,8 +114,48 @@ export const mutations = { | |||
Object.fromEntries(p_columns.map((column) => [column, null])); | |||
}, | |||
|
|||
initializeDataDictionary(p_state) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took me a minute to look over this despite knowing what it's supposed to be doing. I think this below is a more readily understandable and simpler implementation of what is being accomplished in this function. (Could still be compressed by one line too.)
p_state.dataDictionary.annotated = {};
let columns = Object.keys(p_state.dataTable[0]);
for ( let index = 0; index < columns.length; index++ ) {
p_state.dataDictionary.annotated[columns[index]] = {"description": ""};
}
What do you think?
Also for future instances of nesting/chaining like this, this formatting would make it clearer to understand.
p_state.dataDictionary.annotated = Object.fromEntries(
Object.keys(p_state.dataTable[0])
.map(columnName => {
return [columnName, {"description": ""}];
});
);
See the difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree the second option is clearer - we could see if this is something the linter can handle for us!
Re the for
vs map
conversation: I can live with both. To me a short map
statement is clearer than a for loop because it has less boilerplate stuff - especially if the goal is to just create another array. This case here would probably make sense to put in a for
loop. In that case, I would say we stick with the for ... of
syntax since that has been around for a while now and is clearer.
Would probably be good to write down some general guidelines on when we go for one over the other since that has come up a couple of times. I'd be OK with using for ... of
unless its a small mapping or the output is another array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I did notice while looking at this again: I'm pretty sure my mutation wouldn't be reactive the way I wrote it so far. So that'll have to change too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say we stick with the for ... of syntax since that has been around for a while now and is clearer
Sounds good and good point.
I'm pretty sure my mutation wouldn't be reactive the way I wrote it so far. So that'll have to change too.
Since dataTable.annotated
is assigned a brand new object, wouldn't this be reactive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think it would because the nested attributes (i.e. the column names) aren't defined initially. I addressed it in my current commit. Think that'll do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would probably be good to write down some general guidelines on when we go for one over the other since that has come up a couple of times. I'd be OK with using for ... of unless its a small mapping or the output is another array.
So yeah - what's happening here is the creation of a new object (for dataDictionary.annotated
) based on the keys of an array dataTable[0]
.
I can include the performativity/computer science of why the loop is preferable as well in that writeup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think it would because the nested attributes (i.e. the column names) aren't defined initially. I addressed it in my current commit. Think that'll do it
Interesting. I'll read more into that. Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@surchs Ahh okay. I see it.
Sometimes you may want to assign a number of properties to an existing object, for example using Object.assign() or _.extend(). However, new properties added to the object will not trigger changes. In such cases, create a fresh object with properties from both the original object and the mixin object:
// instead of Object.assign(this.someObject, { a: 1, b: 2 })
this.someObject = Object.assign({}, this.someObject, { a: 1, b: 2 })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go!
@@ -100,8 +114,48 @@ export const mutations = { | |||
Object.fromEntries(p_columns.map((column) => [column, null])); | |||
}, | |||
|
|||
initializeDataDictionary(p_state) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think it would because the nested attributes (i.e. the column names) aren't defined initially. I addressed it in my current commit. Think that'll do it
Interesting. I'll read more into that. Looks good!
OK, now this is approved, I'll clean it up a little and then merge it so we keep the (collapsed) commits for the new mutations |
- overwrites existing dataDictionary - preserves Vue reactivity
37b7c93
to
b5a5438
Compare
Closes #256
This PR adds:
processDataTable
action (create action to save the uploaded dataTable to the store #256)setDataTable
to process and then store a provided dataTableinitializeDataDictionary
to create a skeleton data dictionary from a stored dataTablewith tests. The two mutations are meant to be called only from inside the store. There isn't anything to prevent a component to call the mutations directly (other than us not doing that).
The test for the action is very limited, it only checks if the correct mutations are being committed. The reason behind this is that because the mutations are already tested via their unit tests, and because they are mocked during the unit test of the action, it would be pretty tricky to test that the action actually has the desired effect on the state. I think what we've realized here is that action tests are somewhere between unit and integration tests.