Show namespace columns in MAF file in Mutation Table component #4163
Conversation
This is not allowed for id fields.
pvannierop
force-pushed
the
namespacecolumns_in_mutationtable
branch
2 times, most recently
from
02d9c70
to
1a7132f
Compare
| this.props.columns && | ||
| !this.props.columns.includes(columnName) | ||
| ) { | ||
| this.props.columns.push(columnName); |
There was a problem hiding this comment.
looks like we are mutating a prop here? that's antipattern. lets discuss.
| ]![ | ||
| columnName | ||
| ] = createCategoricalFilter((d: Mutation) => | ||
| CategoricalNamespaceColumnFormatter.download( |
There was a problem hiding this comment.
maybe a comment explaining why we are using download? i mean, the problem is perhaps that the "download" is too specific
There was a problem hiding this comment.
Download here means the value formatted for download. I just copied this from other implementations.
| @@ -173,6 +176,24 @@ export default class ResultsViewMutationTable extends MutationTable< | |||
| }; | |||
| } | |||
|
|
|||
| // generate namespace columns | |||
| const namespaceColumns = createNamespaceColumns( | |||
There was a problem hiding this comment.
again it seems odd that we accept a prop of namespace columns only the mutate the columns prop. seems like we should be doing this where the columns prop is prepared?
| return {}; | ||
| } | ||
| const namespaceConfig: NamespaceColumnConfig = {}; | ||
| const nameSpaces = _.flatMap(mutations, m => _.keys(m.namespaceColumns)); |
There was a problem hiding this comment.
it seems strange here that something of type Mutation[] would have something called "namespaceColumns" on it
There was a problem hiding this comment.
I do not see why that would be strange. Maybe namespaceValues would be better. But would we go through the motions in the frontend and backend to update this?
There was a problem hiding this comment.
Again not necessary to address in this PR but what I object to is A) having something called "column" on Mutation type. And "namespace". Shouldn't this be more generic? Like, customValues or something. The fact that they belong to namespaces seems like a property of the individual values themselves. Again, I know this is very late to the game and I am not suggesting we make a change, but worth bringing up. Adding @inodb here. Is this THE one and only way we allow people to extend the Mutation data?
pvannierop
force-pushed
the
namespacecolumns_in_mutationtable
branch
from
1a7132f
to
681b3e0
Compare
| @@ -222,6 +226,31 @@ export default class ResultsViewMutationMapperStore extends MutationMapperStore | |||
| } | |||
| }); | |||
|
|
|||
| // Add numerical namespace column definitions. | |||
There was a problem hiding this comment.
not necessary for this PR but this great example of something that should be in utility function and not bloating store
This is a redo from PR #4159. There were breaking bugs in this and it was reverted. Here regression tests are added for mutation table filtering.
Description
This PR will show custom columns in the MAF file imported with the namespace import mechanism in Mutation Table components of Results View, Patient View and Standalone Mutation Mapper.
Comment
Video
Tests