-
Notifications
You must be signed in to change notification settings - Fork 48
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
Add LollipopGraph #417
Add LollipopGraph #417
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is perfect, except that it should return an immutable digraph, so you need to update that and update the documentation.
Currently one of the PR status checks is failing: the new 'codecov/patch' one. This means you've added or changed code that isn't correspondingly covered by tests. You can see the bits of your added/changed code that don't have code coverage by following the "Details" link next to the failing In particular, that link will tell you "Added line #L402 was not covered by tests". You can also see this information by looking for 'codecov' comments on the diff: https://github.com/digraphs/Digraphs/pull/417/files |
Co-authored-by: Wilf Wilson <wilf@wilf-wilson.net>
Co-authored-by: Wilf Wilson <wilf@wilf-wilson.net>
Co-authored-by: Wilf Wilson <wilf@wilf-wilson.net>
Co-authored-by: Wilf Wilson <wilf@wilf-wilson.net>
Co-authored-by: Wilf Wilson <wilf@wilf-wilson.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is looking good! As with the other PRs, there will unfortunately be merge conflicts to resolve when you next update with the master
branch. Let me know if that's trouble.
Otherwise, I have some requests about the documentation, and some suggestions for further properties to set about the lollipop graph.
Co-authored-by: Wilf Wilson <wilf@wilf-wilson.net>
Co-authored-by: Wilf Wilson <wilf@wilf-wilson.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed some things that got broken by the merge conflicts, and also I addressed my remaining requested changes from #417 (comment) which you had already marked as resolved 😉 . I think this can be merged if the tests pass for a final time.
Ooops I'm so sorry! I don't know how I did this! Apologies! :) |
No description provided.