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

feature(prost-build): Generate less boxed if nested type is boxed manually #1160

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ldm0
Copy link
Contributor

@ldm0 ldm0 commented Sep 17, 2024

Address #1157 (comment)

This branch is based on #1157.

@ldm0 ldm0 force-pushed the boxed_break_nested branch from eecd233 to 13f0b78 Compare September 20, 2024 10:36
Copy link
Collaborator

@caspermeijn caspermeijn left a comment

Choose a reason for hiding this comment

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

That is an interesting problem to fix.

  • Is nested the correct term to use? I would use recursive, but I am not proficient in graph theory.
  • I would like user documentation about how this works.
  • This is a breaking change, so it has to wait for the next breaking release.

@@ -154,3 +165,108 @@ impl MessageGraph {
}
}
}

/// Check two nodes is connected with edge filter
fn is_connected_with_edge_filter<F, N, E>(
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 very nifty, but it is not the core strength of prost. I understand what the function is supposed to do, but I don't understand how it works. This should either be documented better or move to petgraph library.


package nesting_complex;

// ----- Directly nested
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to have more documentation explaining how this structure is nested.

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

Successfully merging this pull request may close these issues.

2 participants