-
Notifications
You must be signed in to change notification settings - Fork 603
Improve C# Doc-Comments to Match C++ and Java #4752
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
Improve C# Doc-Comments to Match C++ and Java #4752
Conversation
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.
Pull request overview
This PR improves C# documentation comments to align with C++ and Java conventions, while also back-porting some C# improvements to the other languages. The changes focus on consistency, clarity, and proper use of XML documentation tags.
Key changes:
- Standardized parameter descriptions to use lowercase and remove terminal periods (Java)
- Updated method descriptions to use active voice ("Returns" vs "Gets", "Notifies" vs "Notification")
- Replaced informal boolean descriptions (
true/false) with proper XML tags (<see langword="true"/>) - Added missing class/interface documentation summaries
- Improved clarity and consistency of technical descriptions across all three languages
Reviewed changes
Copilot reviewed 66 out of 66 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/OutgoingResponse.java | Standardized parameter descriptions to lowercase without periods |
| java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/OperationNotExistException.java | Changed "cannot" to "could not" for grammatical consistency |
| java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/ObjectNotExistException.java | Changed "cannot" to "could not" for grammatical consistency |
| java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/MarshaledResult.java | Updated class description to clarify it's an interface |
| java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/LoggerPlugin.java | Added code formatting to parameter reference |
| java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/Instrumentation/Observer.java | Changed to active voice ("Notifies" vs "Notification of") |
| java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/IncomingRequest.java | Added "object" clarification to "target object" |
| java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/FacetNotExistException.java | Changed "cannot" to "could not" for consistency |
| java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/Current.java | Improved documentation clarity for encoding, request ID, and connection fields |
| java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/CompositeSliceLoader.java | Changed "Slice loaders" to "initial Slice loaders" for clarity |
| java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/Communicator.java | Updated to use singular form and improved method descriptions |
| csharp/src/IceBox/Service.cs | Completely rewrote documentation to be more precise and helpful |
| csharp/src/Ice/Value.cs | Added class summary and improved method documentation |
| csharp/src/Ice/UserException.cs | Added "all" to base class description |
| csharp/src/Ice/UnknownSlicedValue.cs | Improved precision in terminology (object vs value vs instance) |
| csharp/src/Ice/TypeExtensions.cs | Fixed XML formatting and removed extra line break |
| csharp/src/Ice/SlicedData.cs | Improved clarity in parameter descriptions |
| csharp/src/Ice/SliceLoader.cs | Formatted summaries consistently and updated language |
| csharp/src/Ice/ServantLocator.cs | Complete rewrite with more detailed and accurate documentation |
| csharp/src/Ice/ProxyIdentityKey.cs | Replaced informal true/false with langword tags |
| csharp/src/Ice/Proxy.cs | Extensive updates to use langword tags and improve clarity |
| csharp/src/Ice/PluginManagerI.cs | Formatted single-line summary to multi-line |
| csharp/src/Ice/Plugin.cs | Major documentation improvements with clearer descriptions |
| csharp/src/Ice/OutputStream.cs | Updated to use langword tags for boolean values |
| csharp/src/Ice/OutgoingResponse.cs | Fixed spacing in XML tags |
| csharp/src/Ice/ObjectAdapter.cs | Extensive documentation improvements including detailed dispatch pipeline description |
| csharp/src/Ice/Object.cs | Updated to use langword tags |
| csharp/src/Ice/NativePropertiesAdmin.cs | Formatted summaries to multi-line |
| csharp/src/Ice/MarshaledResult.cs | Added complete interface documentation |
| csharp/src/Ice/LoggerPlugin.cs | Minor wording improvement |
| csharp/src/Ice/Logger.cs | Improved formatting and moved remark |
| csharp/src/Ice/LocalExceptions.cs | Improved exception descriptions and updated to use langword tags |
| csharp/src/Ice/LocalException.cs | Improved base class description |
| csharp/src/Ice/Internal/ProtocolPluginFacade.cs | Formatted summary to multi-line |
| csharp/src/Ice/Internal/ObserverMiddleware.cs | Formatted summary to multi-line |
| csharp/src/Ice/Internal/LoggerMiddleware.cs | Formatted summary to multi-line |
| csharp/src/Ice/Internal/LazySliceLoader.cs | Formatted summary to multi-line |
| csharp/src/Ice/Internal/DictionaryExtensions.cs | Formatted summaries and updated to use langword tags |
| csharp/src/Ice/Instrumentation.cs | Improved observer documentation and formatting |
| csharp/src/Ice/InputStream.cs | Updated to use langword tags |
| csharp/src/Ice/InitializationData.cs | Complete rewrite for clarity and conciseness |
| csharp/src/Ice/IncomingRequest.cs | Improved property descriptions |
| csharp/src/Ice/ImplicitContext.cs | Added interface summary and improved method docs |
| csharp/src/Ice/FormatType.cs | Improved enum documentation |
| csharp/src/Ice/Exception.cs | Enhanced base class documentation |
| csharp/src/Ice/EndpointSelectionType.cs | Improved enum descriptions |
| csharp/src/Ice/Endpoint.cs | Comprehensive documentation improvements with issues found |
| csharp/src/Ice/CurrentExtensions.cs | Minor formatting and language improvements |
| csharp/src/Ice/Current.cs | Improved parameter documentation |
| csharp/src/Ice/ConnectionI.cs | Formatted summary to multi-line |
| csharp/src/Ice/Connection.cs | Extensive documentation improvements |
| csharp/src/Ice/CompositeSliceLoader.cs | Formatted summaries and improved descriptions |
| csharp/src/Ice/Communicator.cs | Major documentation overhaul with class summary |
| csharp/src/Ice/BatchRequest.cs | Improved interface and method documentation |
| cpp/include/Ice/MarshaledResult.h | Updated comment with formatting issue |
| cpp/include/Ice/LocalExceptions.h | Changed "cannot" to "could not" for consistency |
| cpp/include/Ice/Instrumentation.h | Minor wording improvement |
| cpp/include/Ice/Initialize.h | Changed "not-null" to "non-null" |
| cpp/include/Ice/IncomingRequest.h | Added "object" clarification |
| cpp/include/Ice/Endpoint.h | Changed "an UDP" to "a UDP" |
| cpp/include/Ice/Current.h | Changed "operation name" to "name of the operation" |
| cpp/include/Ice/Connection.h | Improved flush method descriptions |
| cpp/include/Ice/Communicator.h | Changed "function" to "method" for consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
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.
Pull request overview
Copilot reviewed 66 out of 66 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pepone
left a comment
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.
Looks good!
| @@ -1310,7 +1330,7 @@ public ObjectPrx ice_compress(bool compress) | |||
| /// Obtains the compression override setting of this proxy. | |||
| /// </summary> | |||
| /// <returns>The compression override setting. If no optional value is present, no override is | |||
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 find this method comment hard to understand
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.
It would probably written before we updated the C# mapping to use ?. And not-set is now simply null.
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.
Updated to The compression override setting. If null is returned, no override is set. Otherwise,...
| /// <item>If this fails, the object adapter tries to find a servant locator for the category component of the | ||
| /// identity. If there is no such servant locator, the object adapter tries to find a servant locator for the | ||
| /// empty category. | ||
| /// <list type="bullet"> |
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.
Is this nested list intentional?
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 list is intentional, it shouldn't be nested though, just one level.
Am I missing something about C# here?
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.
Is this list not nested inside the outer one?
/// <list type="bullet">
/// <item>The object adapter tries to find a servant for the identity and facet in the Active Servant Map.</item>
/// <item>If this fails, the object adapter tries to find a default servant for the category component of the
/// identity.</item>
/// <item>If this fails, the object adapter tries to find a default servant for the empty category, regardless of
/// the category contained in the identity.</item>
/// <item>If this fails, the object adapter tries to find a servant locator for the category component of the
/// identity. If there is no such servant locator, the object adapter tries to find a servant locator for the
/// empty category.
/// <list type="bullet">Collapse comment
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.
Ah, I was looking at the list above this one.
This is intentional, to match the list we have in C++:
...
/// - If this fails, the object adapter tries to find a servant locator for the category component of the
/// identity. If there is no such servant locator, the object adapter tries to find a servant locator for the
/// empty category.
/// - If a servant locator is found, the object adapter tries to find a servant using this servant locator.
/// - If all the previous steps fail, the object adapter gives up and the caller receives an
/// ObjectNotExistException or a FacetNotExistException.
...
Could probably just dedent the tick-mark and not bother with a nested list though?
This PR implements #4661 by updating and improving the C# doc-comments,
and to bring them inline with the C++ and Java doc-comments.
It also makes some smaller fixes to C++ and Java where there were typos, or where the C# doc-comment had something I felt should be 'back-ported' to the other language's comments.