Major overhaul and tree converting #39
Conversation
There was a problem hiding this comment.
In general, I think this is all great. I really appreciate your contributions!
One generic comment, I typically do my best to adhere to Microsoft's coding standards and have noticed you use a lot of arrays and List in your methods and properties. This violates CA1002 and CA1819.
I haven't been able to figure out a way to run Code Analysis in .NET Core projects yet, but until I do, I'll still try to give feedback according to the rules I care about most. I'll at least spend some time adding come Code Style definition files to encourage uniformity there.
Get these small things updated when you get a chance and I'll create a new release.
Thanks again!
|
|
||
| namespace Gremlin.Net.CosmosDb.Serialization | ||
| { | ||
| internal class AutoConnector : IVertexConnector, IEdgeConnector |
There was a problem hiding this comment.
Can you do me a favor and add some comments throughout this class and methods just to help future devs?
|
|
||
| foreach (var prop in vertexProps) | ||
| { | ||
| if (prop.GetValue(edge) is object newEdgeVertex) |
There was a problem hiding this comment.
NIT: Using a switch here may be a bit cleaner than all the if/is checks you're doing:
switch (prop.GetValue(edge))
{
case IVertex v:
ConnectVertex(existingEdge, v);
break;
case IEnumerable vEnum:
foreach (var vert in vEnum.OfType<IVertex>())
{
ConnectVertex(existingEdge, vert);
}
break;
}| /// <summary> | ||
| /// The list of Out vertices. | ||
| /// </summary> | ||
| public List<TOutV> OutV { get; set; } |
There was a problem hiding this comment.
This should probably be IReadOnlyCollection<TOutV> or at least IList<TOutV>.
| /// <summary> | ||
| /// The list of In vertices. | ||
| /// </summary> | ||
| public List<TInV> InV { get; set; } |
| /// <summary> | ||
| /// The list of Out vertices. | ||
| /// </summary> | ||
| public List<TOutV> OutV { get; set; } |
| /// <summary> | ||
| /// The list of In vertices. | ||
| /// </summary> | ||
| public List<TInV> InV { get; set; } |
| /// <typeparam name="T">The type to convert to.</typeparam> | ||
| /// <param name="treeConnectors">Custom connectors to use for attaching edges and vertices.</param> | ||
| /// <returns></returns> | ||
| public T[] ToObject<T>(List<ITreeConnector> treeConnectors = null) |
There was a problem hiding this comment.
Can you please change this signature to be the following
public IEnumerable<T> ToObject<T>(IEnumerable<ITreeConnector> treeConnectors = null)
BenjaBobs
force-pushed
the
TreeParser
branch
from
c5eff12 to
4db758c
Compare
* Added comments to AutoConnector * Made the default edges use IList instead of List * Changed the signature of Tree
BenjaBobs
force-pushed
the
TreeParser
branch
from
4db758c to
489300e
Compare
|
I addressed the pr issues and found (and fixed) a small error. |
See #38 for a description.