Do not update dimension when it already supports unit to avoid fraction/dimensionless collision #1515
Conversation
|
Build is failing |
|
Don't forget to reference the issue to close in the commit message #1499 |
| .FirstOrDefault(x => x.HasUnit(column.Unit.SelectedUnit)); | ||
| if (column.Dimension != null && !column.Dimension.HasUnit(column.Unit.SelectedUnit)) | ||
| column.Dimension = _columnInfos | ||
| .First(x => x.DisplayName == model.MappingName) |
There was a problem hiding this comment.
it's in the place where you used the cache that you just implemented?
Also please add a { } around the if when what comes after is over multiple lines
There was a problem hiding this comment.
You are right. It was on the other PR but it was already merged so I am rebasing to include the changes
| column.Dimension = _columnInfos | ||
| .First(x => x.DisplayName == model.MappingName) | ||
| .SupportedDimensions | ||
| .FirstOrDefault(x => x.HasUnit(column.Unit.SelectedUnit)); |
There was a problem hiding this comment.
I see this code over an over . FirstOrDefault(x=>x.HasUnit). It needs to be refactored at some point. I am pointing out here because I am seeing it. Please create an issue
There was a problem hiding this comment.
it's always coming from the SupportedDimensions. If we make it a real class as opposed to a list? Or it belongs to the Columninfo itself. Not that Supporteddimension is a List and not a IReadOnlyList which breaks encapsulation
| @@ -41,6 +42,22 @@ public static IDimension ConcentrationDimensionForSpecs() | |||
| return _concentrationDimension; | |||
| } | |||
|
|
|||
| public static IEnumerable<IDimension> ExtendedDimensionsForSpecs() | |||
| { | |||
| var dimensions = new List<IDimension>(); | |||
There was a problem hiding this comment.
Why aren't you using the dimensions already defined in the file (concentration, fraction etc..).
The NO_Dimension already exists also as a dImension normally so you code could be something like
reutnr new []{COnentrationDimForSPecs, NO_DIMENSION, FractionDimForSpecs).
| _mappingSource = _parameters[2] as MappingDataFormatParameter; | ||
| _mappingSource.MappedColumn.Dimension = supportedDimensions.First(x => x.Name == Constants.Dimension.FRACTION); | ||
| A.CallTo(() => _mappingParameterEditorPresenter.Unit).Returns(new UnitDescription("")); | ||
| A.CallTo(() => _mappingParameterEditorPresenter.SelectedErrorType).Returns(1); |
There was a problem hiding this comment.
What does this number means? 1
There was a problem hiding this comment.
I have created some named constants
…on/dimensionless collision
f8ed534 to
c5d36eb
Compare
| @@ -168,7 +170,7 @@ protected override void Context() | |||
| { | |||
| base.Context(); | |||
| A.CallTo(() => _mappingParameterEditorPresenter.Unit).Returns(new UnitDescription("")); | |||
| A.CallTo(() => _mappingParameterEditorPresenter.SelectedErrorType).Returns(0); | |||
| A.CallTo(() => _mappingParameterEditorPresenter.SelectedErrorType).Returns(GEOMETRIC_ERROR); | |||
There was a problem hiding this comment.
I think this should return the enum that we have already defined as opposed to number. I am going to accept this PR but create an issue for this because returning the index of the combo box is not ok
Add missing {}
Fixes #1499 (Do not update dimension when it already supports unit to avoid fraction/dimensionless collision)