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

Add PushError::LimitExceeded. #349

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ximon18
Copy link
Member

@ximon18 ximon18 commented Jul 9, 2024

As discussed in the comments on #348 this PR introduces a new PushError variant for the limit exceeded case.

This was not done in the original PR as it is a breaking change.

ximon18 added 3 commits July 9, 2024 14:39
This is useful when wanting to "reserve" space such that pushes at one point in the code to the message will leave behind enough room for subsequent pushes later, e.g. of OPT or TSIG RRs.
Note: This is a breaking change.
@ximon18 ximon18 added the breaking A PR that includes a breaking change. label Jul 9, 2024
@ximon18 ximon18 requested a review from a team July 9, 2024 13:26
@ximon18 ximon18 changed the base branch from add-push-limit-to-message-builder to main July 9, 2024 13:35
Copy link
Contributor

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

Looks like a good change to me, just one comment about some stuff that was already there.

@@ -2332,6 +2332,7 @@ impl<Target: Composer> Truncate for TreeCompressor<Target> {
#[derive(Clone, Copy, Debug)]
pub enum PushError {
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nice to document these variants to explain what they mean in rustdoc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A PR that includes a breaking change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants