1374 create configuration for should store dimension #1382
Conversation
| if (mappingColumn?.Unit == null || mappingColumn.Dimension != null) | ||
| continue; | ||
|
|
||
| var concreteColumnInfo = columnInfos.FirstOrDefault(x => x.Name == parameter.MappedColumn.Name); |
There was a problem hiding this comment.
used mappingColumn instead of parameter.MappedColumn
| { | ||
| var mappingColumn = parameter.MappedColumn; | ||
|
|
||
| if (mappingColumn?.Unit == null || mappingColumn.Dimension != null) |
There was a problem hiding this comment.
why mappingColumn? in the first part of the if but not in the second? this is not logical. Either you fear that mappingColumn is null or you don't fear. But it should always be the same behavior
There was a problem hiding this comment.
If mappingColumn (I have just renamed it to mappedColumn, I think it makes more sense) is null, then the first part will be true and then the second part will not be evaluated. But I agree it is not fully logic to have it only on the first part. I will add it to the second part even though it is totally useless.
| if (mappingColumn?.Unit == null || mappingColumn.Dimension != null) | ||
| continue; | ||
|
|
||
| var concreteColumnInfo = columnInfos.FirstOrDefault(x => x.Name == parameter.MappedColumn.Name); |
There was a problem hiding this comment.
FirstOrDefault means thius cam be null. Or you do not check for null. Therefor either a check is missing or it should be First
There was a problem hiding this comment.
True, I changed it to First since null will mean the configuration is wrong and it should actually throw.
| mappingColumn.Unit.SelectedUnit == UnitDescription.InvalidUnit) | ||
| { | ||
| mappingColumn.Dimension = concreteColumnInfo.DefaultDimension; | ||
| mappingColumn.Unit = new UnitDescription(mappingColumn.Dimension.BaseUnit.Name); |
There was a problem hiding this comment.
Is this supposed to be the baseUnit or the DefaultUnit? We also have a DEfaultUnitName property directly
There was a problem hiding this comment.
Right, DefaultUnitName should be the one.
| @@ -109,53 +109,9 @@ private void setDataFormat(IList<DataFormatParameter> formatParameters) | |||
| View.SetMappingSource(_mappings); | |||
| ValidateMapping(); | |||
| InitializeErrorUnit(); | |||
| setDimensionsForMappings(); | |||
| //setDimensionsForMappings(); | |||
| @@ -60,7 +60,7 @@ private void setDimensionsForMappings(IReadOnlyList<ColumnInfo> columnInfos) | |||
| if (mappedColumn?.Unit == null || mappedColumn?.Dimension != null) | |||
| continue; | |||
|
|
|||
| var concreteColumnInfo = columnInfos.First(x => x.Name == mappedColumn.Name); | |||
| var concreteColumnInfo = columnInfos.First(x => x.DisplayName == mappedColumn.Name); | |||
There was a problem hiding this comment.
What's the displayName of a column Info? Do we have instances where it's not the same as Name? I think this makes sense to use DisplayName but I don't know why we have this in the first place
Fixes #1374 (CreateConfigurationFor should store dimension)