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

Enables vertices to have more than one label #4

Merged

Conversation

MachinesAreUs
Copy link
Contributor

@MachinesAreUs MachinesAreUs commented Sep 1, 2017

I know it's possible to assign complex labels to vertices, like [:foo, :bar], but it would be nice if the handling of multiple labels could be internal to the library, since in several use cases you don't know beforehand all the labels for vertices.

This could work like this (modified and extended from doctests for Graph.label_vertex/3):

iex> g = Graph.new |> Graph.add_vertex(:a, :foo)
...> [:foo] = Graph.vertex_labels(g, :a)
...> g = Graph.label_vertex(g, :a, :bar)
...> Graph.vertex_labels(g, :a)
[:foo, :bar]

iex> g = Graph.new |> Graph.add_vertex(:a, [:foo, :bar])
...> Graph.vertex_labels(g, :a)
[:foo, :bar]

You could also receive both single terms and lists in Graph.label_vertex/3:

iex> g = Graph.new |> Graph.add_vertex(:a)
...> g = Graph.label_vertex(g, :a, [:foo, :bar])
...> Graph.vertex_labels(g, :a)
[:foo, :bar]

This implies 2 breaking changes:

  • The s added at the end of the original Graph.vertex_label/2.
  • Graph.vertex_labels/2 always returns a list.

I was careful not to break the .dot serialization (at least with the current tests), but maybe it is worth discussing including both the vertex name and labels in the generated .dot file.

@coveralls
Copy link

coveralls commented Sep 1, 2017

Coverage Status

Coverage decreased (-0.1%) to 94.775% when pulling 0d47b62 on MachinesAreUs:feature/multiple_labels_by_vertex into 391b24d on bitwalker:master.

@MachinesAreUs MachinesAreUs force-pushed the feature/multiple_labels_by_vertex branch from 0d47b62 to 9b83eb7 Compare September 1, 2017 04:29
@coveralls
Copy link

coveralls commented Sep 1, 2017

Coverage Status

Coverage decreased (-0.1%) to 94.775% when pulling 9b83eb7 on MachinesAreUs:feature/multiple_labels_by_vertex into 391b24d on bitwalker:master.

@MachinesAreUs MachinesAreUs force-pushed the feature/multiple_labels_by_vertex branch from 9b83eb7 to c79b5c0 Compare September 1, 2017 04:45
@coveralls
Copy link

coveralls commented Sep 1, 2017

Coverage Status

Coverage increased (+0.06%) to 94.946% when pulling c79b5c0 on MachinesAreUs:feature/multiple_labels_by_vertex into 391b24d on bitwalker:master.

@MachinesAreUs MachinesAreUs force-pushed the feature/multiple_labels_by_vertex branch from c79b5c0 to 74733e2 Compare September 1, 2017 05:00
@coveralls
Copy link

coveralls commented Sep 1, 2017

Coverage Status

Coverage increased (+0.06%) to 94.946% when pulling 74733e2 on MachinesAreUs:feature/multiple_labels_by_vertex into 391b24d on bitwalker:master.

@MachinesAreUs
Copy link
Contributor Author

Hi @bitwalker. Any thoughts on this? :)

@bitwalker
Copy link
Owner

Sorry for the delay, I promise this is next on my list! Hopefully review this weekend and go from there :)

@bitwalker
Copy link
Owner

Can you rebase your changes on master? There are some changes in your PR that I've made on master, or that I would prefer not be in PRs (such as the version change), but other than that, it's pretty much ready to merge. Sorry for the long wait!

@MachinesAreUs MachinesAreUs force-pushed the feature/multiple_labels_by_vertex branch from 74733e2 to 663b853 Compare October 4, 2017 15:18
@coveralls
Copy link

coveralls commented Oct 4, 2017

Coverage Status

Coverage increased (+0.06%) to 94.955% when pulling 663b853 on MachinesAreUs:feature/multiple_labels_by_vertex into 3851394 on bitwalker:master.

@MachinesAreUs
Copy link
Contributor Author

Ready @bitwalker ! :)

@bitwalker bitwalker merged commit e94b118 into bitwalker:master Oct 4, 2017
@bitwalker
Copy link
Owner

Thanks! Since this is a breaking change, I'll release it as 1.0.0 sometime today.

@MachinesAreUs MachinesAreUs deleted the feature/multiple_labels_by_vertex branch October 4, 2017 17:36
heydtn pushed a commit to heydtn/libgraph that referenced this pull request Nov 1, 2019
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.

3 participants