Github user ashwini-ms commented on the issue:
https://github.com/apache/tinkerpop/pull/868
@jorgebay and
@FlorianHockmann<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FFlorianHockmann&data=02%7C01%7Cashwsing%40microsoft.com%7C856d53d68d0a4bfad4ea08d5c5ff16c8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636632621740995185&sdata=ITQC9ta9GUYUWCx8%2BAeEtspe1Innqo8rsolRlGR1olY%3D&reserved=0>
, Thanks for taking a look at this. I will update the review comment soon.
Thanks
Ashwini
From: Jorge Bay Gondra <[email protected]>
Sent: Wednesday, May 30, 2018 12:30 AM
To: apache/tinkerpop <[email protected]>
Cc: Ashwini Singh <[email protected]>; Mention
<[email protected]>
Subject: Re: [apache/tinkerpop] Tinkerpop 1913 (#868)
@jorgebay commented on this pull request.
Its looking good to me, I've included a couple of remark below.
I agree with
@FlorianHockmann<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FFlorianHockmann&data=02%7C01%7Cashwsing%40microsoft.com%7C856d53d68d0a4bfad4ea08d5c5ff16c8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636632621740995185&sdata=ITQC9ta9GUYUWCx8%2BAeEtspe1Innqo8rsolRlGR1olY%3D&reserved=0>
that IGremlinClient should return instances of ResultSet<T>.
Going one step further, we should consider exposing IResultSet<T> and hide
the actual implementation.
________________________________
In
gremlin-dotnet/src/Gremlin.Net/Driver/ResultSet.cs<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Ftinkerpop%2Fpull%2F868%23discussion_r191660636&data=02%7C01%7Cashwsing%40microsoft.com%7C856d53d68d0a4bfad4ea08d5c5ff16c8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636632621740995185&sdata=39hJ6TQ9W5fI%2FLWFHqbAO90zIiLv6KerR3B8uKVZ3U4%3D&reserved=0>:
> + /// <summary>Returns an enumerator that iterates through a
collection.</summary>
+ /// <returns>An <see cref="T:System.Collections.IEnumerator" />
object that can be used to iterate through the collection.</returns>
+ IEnumerator IEnumerable.GetEnumerator()
+ {
+ return this.GetEnumerator();
+ }
+
+ /// <summary>Gets the number of elements in the
collection.</summary>
+ /// <returns>The number of elements in the collection. </returns>
+ public int Count => this.Data.Count;
+ }
+
+ /// <summary>
+ /// Extension for IReadOnlyCollection
+ /// </summary>
+ public static class ResultSetExtensions
Can we hide this from the public API?
________________________________
In
gremlin-dotnet/src/Gremlin.Net/Driver/Connection.cs<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Ftinkerpop%2Fpull%2F868%23discussion_r191665300&data=02%7C01%7Cashwsing%40microsoft.com%7C856d53d68d0a4bfad4ea08d5c5ff16c8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636632621741005190&sdata=k4WDPT9T0kDODzmtr%2F9riB59fTLa7XJ09oNgmXDmHbg%3D&reserved=0>:
> @@ -114,11 +116,18 @@ internal class Connection : IConnection
result.Add(d);
}
}
+
+ if (status.Code == ResponseStatusCode.Success ||
status.Code == ResponseStatusCode.NoContent)
+ {
+ statusAttributes = receivedMsg.Status.Attributes;
Anything we expose should be agnostic to the serialization format. Status
attributes should not be dependent on how GraphSON3 composes the response.
In this case, we are exposing a dictionary containing @type and @value,
where @value is a JArray (a third party type).
Furthermore, we should hide any third party types from our API, see
TINKERPOP-1696 for example.
â
You are receiving this because you were mentioned.
Reply to this email directly, view it on
GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Ftinkerpop%2Fpull%2F868%23pullrequestreview-124271408&data=02%7C01%7Cashwsing%40microsoft.com%7C856d53d68d0a4bfad4ea08d5c5ff16c8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636632621741005190&sdata=VUpaUzlw8Ei%2FLXyVCWcBW3P5LwS6C4zDEjEN9ZMKU4M%3D&reserved=0>,
or mute the
thread<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAdcfc3cynNQU68Dpr6Dsn0nCyaBjSrT4ks5t3kpdgaJpZM4T-UoQ&data=02%7C01%7Cashwsing%40microsoft.com%7C856d53d68d0a4bfad4ea08d5c5ff16c8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636632621741015194&sdata=DRRufkXAqep19e%2F9QFMLqiwhaDXOoPkv3BH0urpk96o%3D&reserved=0>.
---
