[GitHub] [tinkerpop] FlorianHockmann commented on a change in pull request #1565: TINKERPOP-2518 Enable more Gherkin scenarios (master)

[GitBox <gi...@apache.org>]

FlorianHockmann commented on a change in pull request #1565:
URL: https://github.com/apache/tinkerpop/pull/1565#discussion_r804654789



##########
File path: gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GraphTraversal.cs
##########
@@ -772,8 +772,16 @@ private GraphTraversal(ICollection<ITraversalStrategy> traversalStrategies, Byte
         /// </summary>
         public GraphTraversal<S, E> HasId (object id, params object[] otherIds)
         {
-            var args = new List<object>(1 + otherIds.Length) {id};

Review comment:
       @mikepersonick This change is basically the same you did in #1557 for the `P` predicates. We have other steps here that also take a `params` array argument and then access its `.Length` without checking whether it's `null` first (e.g., the `HasKey` step below).
   As I mentioned in the PR description, I'm not sure whether we want to allow `null` everywhere or maybe throw an `ArgumentNullException` in some places where we're sure that `null` isn't a valid argument.
   Any thoughts on that?
   
   Otherwise, I'd suggest that we create a ticket for this issue and then address it in a follow-up PR where we can concentrate on just that topic.




-- 
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: commits-unsubscribe@tinkerpop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org