Skip to content

Conversation

jbaross-pometry
Copy link
Contributor

What changes were proposed in this pull request?

Further changes required

  • The new itterables need documenting, I have not been able to identify where this should be done.
  • Some additional types, such as MetadataView should also be exposed.
  • value should either be exposed or the documented return type chnaged if there is already a Python equivilent (I was not able to identify one).

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'GraphQL Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: 59feaed Previous: f14de7d Ratio
addNode 20 req/s 1460 req/s 73

This comment was automatically generated by workflow using github-action-benchmark.

Returns the source node of the edge.

Returns:
GqlNode:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think this should be Node

/// Min result.
///
/// Returns:
/// value:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a Proptype i.e. if the property is a float it would be a float

/// Returns the source node of the edge.
///
/// Returns:
/// Nodes:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit unfortunte as for Edge this is a Node and for Edges this is a PathFrom I believe?

/// Returns the id of the edge.
///
/// Returns:
/// GID:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a GID pair?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// Explodes an edge and returns all instances it had been updated as separate edges
///
/// Returns:
/// Exploded:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be Edges I believe

fn explode(&self) -> Self::Exploded;

/// Returns:
/// Exploded:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

edges

/// DateTime:
fn date_time(&self) -> Self::ValueType<Option<DateTime<Utc>>>;

/// Gets the layer name for the edge if it is restricted to a single layer
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the below not need return types as well?

})
}

/// Returns the number of times a change to the history was made.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returns int?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these not exposed in python?

///
/// Returns:
/// DegreeView: a view of the undirected node degrees
/// DegreeView: a view of the undirected node degrees.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will all of these Nodestates we are going to want to say these return a nodestate

///
/// Returns:
/// DataFrame: the view of the node data as a pandas Dataframe
/// DataFrame: the view of the node data as a pandas Dataframe.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need to be pandas.dataframe

/// Parameters:
/// file_path: (str)
///
/// Returns:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Errors?

/// path: (str)
/// subset: (bool)
///
/// Returns:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errors?

/// Explodes returns an edge object for each update within the original edge.
///
/// Returns:
/// Exploded:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not nodes above or exploded here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants