Improvement Number Format #13744
Improvement Number Format #13744
Conversation
|
|
| } | ||
| } | ||
|
|
||
| private bool IsDoubleRealNumber(string valueToTest) |
There was a problem hiding this comment.
Do we really need this function? Since this is only a one line if check, maybe we can just fold it to the function above?
if (double.TryParse(valueToTest, out double d) && !Double.IsNaN(d) && !Double.IsInfinity(d))
{
return Convert.ToDouble(numberValue).ToString(nodeViewModel.DynamoViewModel.PreferenceSettings.NumberFormat);
}
else
{
return numberValue;
}
There was a problem hiding this comment.
Do we really need this function? Since this is only a one line if check, maybe we can just fold it to the function above?
if (double.TryParse(valueToTest, out double d) && !Double.IsNaN(d) && !Double.IsInfinity(d)) { return Convert.ToDouble(numberValue).ToString(nodeViewModel.DynamoViewModel.PreferenceSettings.NumberFormat); } else { return numberValue; }
just wanted to keep separate it, probably it would be part of a helper class or similar so we can move latter, like ADSK.Dynamo.Helpers.IsDoubleRealNumber (maybe we already have a similar) or you approach to keep it simple is totally valid, please your thoughts.
There was a problem hiding this comment.
OK, in that case, I am fine with keeping it, but we should then probably move this function to one of DynamoUtilites file, like https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCoreWpf/Utilities/PreferencesPanelUtilities.cs. Also maybe pick a name easier to understand, like IsValidDoubleNumber?
QilongTang
left a comment
There was a problem hiding this comment.
LGTM with two comments
|
@jesusalvino I think there was some work done on this before and this issue is not isolated to only I feel we should consider that previous work before merging this fix that is specific to just one node. |
|
@aparajit-pratap I have picked up your previous work and refactored the mine based on your suggestion in my last commit 584cbcb , your thoughts please. |
| if (Data is double) | ||
| { | ||
| return (Data as IFormattable).ToString(PrecisionFormat, CultureInfo.InvariantCulture); | ||
| } | ||
| else |
There was a problem hiding this comment.
I would comment out this code for the time being and leave a TODO comment here saying "uncomment this once https://jira.autodesk.com/browse/DYN-5101 is complete".
| default: | ||
| if (double.TryParse(obj.ToString(), out double d)) | ||
| { | ||
| return Convert.ToDouble(obj).ToString(PrecisionFormat, CultureInfo.InvariantCulture); |
There was a problem hiding this comment.
This should also be ProtoCore.Mirror.MirrorData.PrecisionFormat.
| return ObjectToLabelString(obj); | ||
| case TypeCode.Double: | ||
| return ((double)obj).ToString(numberFormat, CultureInfo.InvariantCulture); | ||
| return ((double)obj).ToString(ProtoCore.Mirror.MirrorData.PrecisionFormat, CultureInfo.InvariantCulture); |
There was a problem hiding this comment.
Again I would comment out this change for the time being until https://jira.autodesk.com/browse/DYN-5101 is done. Add a TODO.
| IsCollection = label == WatchViewModel.LIST || label == WatchViewModel.DICTIONARY; | ||
| } | ||
|
|
||
| internal static string PrecisionFormat { get; set; } = "f3"; |
There was a problem hiding this comment.
I don't think we need to add this property.
| nodeViewModel.NodeModel.OutPorts.Select(p => p.Name).Where(n => !string.IsNullOrEmpty(n)); | ||
| } | ||
|
|
||
| WatchViewModel.PrecisionFormat = nodeViewModel.DynamoViewModel.PreferenceSettings.NumberFormat; |
There was a problem hiding this comment.
Changes to this file can be reverted entirely.
| @@ -1,4 +1,5 @@ | |||
| using System.Linq; | |||
| using System; | |||
| using System.Linq; | |||
There was a problem hiding this comment.
This file can be reverted entirely.
|
Reported regression is sporadic |
Purpose
Implement the improvement https://jira.autodesk.com/browse/DYN-4469
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@zeusongit
FYIs
@RobertGlobant20 @filipeotero