FlorianHockmann commented on code in PR #1808:
URL: https://github.com/apache/tinkerpop/pull/1808#discussion_r970729973
##########
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GraphTraversal.cs:
##########
@@ -141,73 +159,73 @@ private GraphTraversal(ICollection<ITraversalStrategy>
traversalStrategies, Byte
/// <summary>
/// Adds the aggregate step to this <see
cref="GraphTraversal{SType, EType}" />.
/// </summary>
- public GraphTraversal<S, E> Aggregate (Scope scope, string
sideEffectKey)
+ public GraphTraversal<S, E_> Aggregate (Scope scope, string
sideEffectKey)
{
Bytecode.AddStep("aggregate", scope, sideEffectKey);
- return Wrap<S, E>(this);
+ return Wrap<S, E_>(this);
Review Comment:
The C# [naming convention
suggests](https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/generics/generic-type-parameters#type-parameter-naming-guidelines)
that names of generic type parameters like these should start with a _T_. We
didn't follow that and instead just reused the names from Java _S_ and _E_, but
I'm wondering if we should simply change these into _TS_ and _TE_ or even
_TStart_ and _TEnd_ to be more descriptive and comply with the naming
convention that most C# devs are used to.
This would then also solve this problem here.
If you don't have any objections, then I'll create a separate issue in Jira
to change these names in a different PR. But this shouldn't block this PR here.
So, I suggest that we keep it for now as you've did in this PR with `E_` and
then we can later change that to `TE` / `TEnd` or whatever.
That's definitely better than changing the name of the step `E()` itself as
that would lead to a lot more confusion in my opinion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
