Skip to content
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.

Sign up for GitHub

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

2-liner-exception-messages-fix #3535

Merged
merged 4 commits into from
Jul 1, 2024
+6 −2
Merged

2-liner-exception-messages-fix #3535

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
838b38c
Update FeatureClassUtils.cs
KatKatKateryna Jun 26, 2024
4129e5b
show CRS name
KatKatKateryna Jun 26, 2024
6caefb3
exception message
KatKatKateryna Jun 26, 2024
d385223
Merge branch 'dui3/alpha' into 1-liner-exception-message-fix
oguzhankoral Jul 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion DUI3-DX/Converters/ArcGIS/Speckle.Converters.ArcGIS3/Geometry/PointToSpeckleConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ is not MapPoint reprojectedPt
)
{
throw new SpeckleConversionException(
$"Conversion to Spatial Reference {_contextStack.Current.Document.Map.SpatialReference} failed"
$"Conversion to Spatial Reference {_contextStack.Current.Document.Map.SpatialReference.Name} failed"
);
}
return new(reprojectedPt.X, reprojectedPt.Y, reprojectedPt.Z, _contextStack.Current.SpeckleUnits);
Expand Down
2 changes: 1 addition & 1 deletion DUI3-DX/Converters/ArcGIS/Speckle.Converters.ArcGIS3/Utils/FeatureClassUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ public ACG.GeometryType GetLayerGeometryType(VectorLayer target)
GISLayerGeometryType.POLYLINE => ACG.GeometryType.Polyline,
GISLayerGeometryType.MULTIPATCH => ACG.GeometryType.Multipatch,
GISLayerGeometryType.POLYGON3D => ACG.GeometryType.Multipatch,
_ => throw new ArgumentOutOfRangeException(nameof(target)),
_ => throw new ArgumentOutOfRangeException($"{originalGeomType}"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the first parameter of ArgumentOutOfRangeException is it a message, or is it an argument name?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it is wanting the parameter name, so nameof(target) was correct no?
You can allways do

new ArgumentOutOfRangeException(nameof(target), "my custom message about what this exception is...");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, true, but parameter name doesn't help the user (e.g. message "System.ArgumentOutOfRangeException: 'Specified argument was out of the range of valid values. (Parameter 'MultiPolygonZ')'" is more helpful than "Parameter 'target' " / "Parameter 'originalGeomType' "

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can also be $"{nameof(target)} = {originalGeomType}"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to have info for both so can we include both in the message and use nameof() in the code, it might be a detail but if we're looking at this exception I want to know which thing in the code was out of range. I also don't have the context, so is the argument out of range or invalid?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BovineOx I eventually added both!

};
}
}
Loading