-
Notifications
You must be signed in to change notification settings - Fork 8
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
Do not accept invalid units #982 #986
Conversation
@@ -39,6 +44,7 @@ protected override UnitDescription ExtractUnits(string description, IUnformatted | |||
.Substring(1, units.Length - 2) //remove the brackets | |||
.Trim() //remove whitespace | |||
.Split(',') //split comma separated list | |||
.Where(unitName => _dimensionFactory.Dimensions.SelectMany(dimension => dimension.Units).Any(unitFromDimension => unitFromDimension.Name == unitName)) //only accepts valid units |
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 do not think this is correct here. We have a lot of methods checking whether a unit is valid or not (taking care of case, synonyms etc...)
This needs to change
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.
Checking of units is not what I expected. Look at existing method
For instance
IDimension DimensionForUnit(string unitName)
if the retruend value is null., it is not a valid dimension.
@@ -39,6 +44,7 @@ protected override UnitDescription ExtractUnits(string description, IUnformatted | |||
.Substring(1, units.Length - 2) //remove the brackets | |||
.Trim() //remove whitespace | |||
.Split(',') //split comma separated list | |||
.Where(unitName => _dimensionFactory.DimensionForUnit(unitName) != null) //only accepts valid units |
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.
The same logic should be used for other unit recognition by the way (in column headeR)
Is this the case @abdelr and @georgeDaskalakis
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.
merging. Please review my comment
Do not accept invalid units #982