Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update README.md #91

Merged
merged 4 commits into from
Oct 23, 2024
Merged

Update README.md #91

merged 4 commits into from
Oct 23, 2024

Conversation

curran
Copy link
Member

@curran curran commented Sep 12, 2024

First pass docs update

First pass docs update
@curran
Copy link
Member Author

curran commented Sep 12, 2024

First pass of README update done largely with AI, passing the old README and changelog.

@JesusTheHun Would you be able to please review this and flag any major issues? Thanks!

README.md Outdated
@@ -23,17 +23,17 @@ This library is distributed only via [NPM](npmjs.com). Install by running
Require it in your code like this.

```javascript
const { Graph } = require('graph-data-structure');
const { Graph, serializeGraph, deserializeGraph, topologicalSort, shortestPath } = require('graph-data-structure');
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's type to use ESM ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great idea! If you make inline suggestions with the "add a suggestion" tool I can commit the changes directly.

image

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
@@ -98,29 +98,29 @@ For more detailed example code that shows more methods, have a look at the [test

Constructs an instance of the graph data structure.

The optional argument _serialized_ is a serialized graph that may have been generated by **[serialize](#serialize)**. If _serialized_ is present, it is deserialized by invoking **[deserialize](#deserialize)**.
The optional argument _serialized_ is a serialized graph that may have been generated by **[serializeGraph](#serializeGraph)**. If _serialized_ is present, it is deserialized by invoking **[deserializeGraph](#deserializeGraph)**.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, to deserialize the graph you have to call deserializeGraph(mySerializedObject). There constructor takes no parameter anymore

README.md Outdated

### Adding and Removing Nodes

<a name="add-node" href="#add-node">#</a> <i>graph</i>.<b>addNode</b>(<i>node</i>)

Adds a node to the graph. Returns _graph_ to support method chaining. The argument _node_ is a string identifier that uniquely identifies the node within this graph instance. If a node with the same identifier was already added to the graph, this function does nothing.
Adds a node to the graph. Returns _graph_ to support method chaining. The argument _node_ can be any object or string that uniquely identifies the node within this graph instance. If a node with the same identifier was already added to the graph, this function does nothing.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove the part on the identifier, as we now accept object, it has no meaning anymore. Or maybe "if the node already exists, this function does nothing"


<a name="remove-node" href="#remove-node">#</a> <i>graph</i>.<b>removeNode</b>(<i>node</i>)

Removes the specified node. Returns _graph_ to support method chaining. The argument _node_ is a string identifier for the node to remove. This function also removes all edges connected to the specified node, both incoming and outgoing.
Removes the specified node. Returns _graph_ to support method chaining. The argument _node_ is a string or object identifier for the node to remove. This function also removes all edges connected to the specified node, both incoming and outgoing.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also add a warning but node that are object. You have to remove them using the exact same reference as when they were created. One can use getNode() to retrieve such reference

- `source` The node identifier string of the source node (**u**).
- `target` The node identifier string of the target node (**v**).
- `source` The node reference of the source node (**u**).
- `target` The node reference of the target node (**v**).
- `weight` The weight of the edge between the source and target nodes.
Copy link
Contributor

Choose a reason for hiding this comment

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

You forgot the edge's properties, if any

<a name="deserialize" href="#deserialize">#</a> <i>graph</i>.<b>deserialize</b>(<i>serialized</i>)

Deserializes the given serialized graph. Returns _graph_ to support method chaining. The argument _serialized_ is a graph representation with the structure described in **[serialize](#serialize)**. This function iterates over the serialized graph and adds the nodes and links it represents by invoking **[addNode](#add-node)** and **[addEdge](#add-edge)**. The output from **[serialize](#serialize)** can be used as the input to **deserialize**.
Deserializes the given serialized graph. Returns a new _graph_. The argument _serialized_ is a graph representation with the structure described in **[serializeGraph](#serializeGraph)**.
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes me think that the serializeGraph could also use an identity function, to avoid the duplication of the data when using a node object.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool idea!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@curran curran marked this pull request as ready for review October 23, 2024 15:40
@curran
Copy link
Member Author

curran commented Oct 23, 2024

OK I went through and addressed most of the feedback. Merging this now for upcoming release.

Please feel free to update the README if you see any issues @JesusTheHun ! Thanks!

@curran curran merged commit ff5e564 into master Oct 23, 2024
1 check passed
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