Skip to content

Conversation

@Cobord
Copy link

@Cobord Cobord commented Oct 16, 2025

  • Fix up some of the doc comments (backticks)
  • On options map and then unwraps are replaced with and_then, is_some_and, ...
  • Be more explicit about casts to be clear when as would have been fallible vs infallible
  • Panics and Error docs in format so they show up as their own sections
  • Direct comparisons with Some(0)... instead of mapping and unwrapping
  • Question about the outputs of .compile() never being used, would expect the Looped to use have Self::Output built from the C::Output(s)
  • Instead of directly accessing graph.graph.node_weight, etc inspecting in fields of the struct have methods to do these common tasks. They are inlined methods on Graph instead.
  • Tests assert in addition to printing. Want it to be testing properly even if don't --capture output
  • A disjoint union of Graph and a test thereof. There are further things to discuss about GraphTensor with this point.

@jafioti
Copy link
Collaborator

jafioti commented Oct 18, 2025

What was cleaned up? Please provide details, there are many lines changed

@Cobord
Copy link
Author

Cobord commented Oct 18, 2025

Not too much to say, but I will do so either tomorrow or Monday after can get back to this. Should have put as draft because didn't get to the description part yesterday before context switching.

…isjoint first and then wire up all at once after that
@Cobord
Copy link
Author

Cobord commented Oct 28, 2025

Any more documentation of what was done that you suggest?

@Cobord
Copy link
Author

Cobord commented Nov 3, 2025

Let me know if there is any more wanted written here. This one is mostly about making more idiomatic. I find that once that happens it is much easier for me to add features without having the linter full of extraneous complaints.

@jafioti
Copy link
Collaborator

jafioti commented Nov 3, 2025

I don't think so, I just havent had a chance to review it yet because a good amount of lines were changed. Should have time today or this week!

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