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

proto: add into_parts methods #1397

Merged
merged 8 commits into from Mar 8, 2021
Merged

Conversation

leshow
Copy link
Contributor

@leshow leshow commented Mar 1, 2021

summary:

  • Header is Copy
    - options_mut added to OPT (I had wanted this before but for some reason forgot to add it)
  • AsMut/AsRef added to OPT
  • into_parts methods added to Message Record Query RData -- these methods are useful for us because we want to consume a Message after we're done processing and transform it into a log struct, however we can only clone data currently.

It occurred to me that it might be nice to have into_ascii that consumes self and returns a String as well as to_ascii for Name and Label. I have not added these here as I'm not sure if it's possible yet. Any thoughts?

@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #1397 (ab0fdd0) into main (61122bc) will decrease coverage by 0.00%.
The diff coverage is 64.71%.

@@            Coverage Diff             @@
##             main    #1397      +/-   ##
==========================================
- Coverage   85.16%   85.16%   -0.00%     
==========================================
  Files         153      153              
  Lines       15040    15036       -4     
==========================================
- Hits        12808    12804       -4     
  Misses       2232     2232              

@leshow
Copy link
Contributor Author

leshow commented Mar 2, 2021

@bluejekyll we had talked about this yesterday, but there is in fact a

#![warn(missing_copy_implementations)]

lint that would have caught the Header stuff. Open to adding this? I like adding a bunch more on top of this:

#![warn(
    missing_copy_implementations,
    missing_debug_implementations,
    missing_docs,
    rust_2018_idioms,
    unreachable_pub,
    non_snake_case,
    non_upper_case_globals
)]

But it is rather subjective

@bluejekyll
Copy link
Member

bluejekyll commented Mar 5, 2021

Most of those warnings look good. Do they not get enabled by default with clippy? As part of our cleanliness test in CI, we run clippy with default settings and -D warnings. I'd be happy for us to add those warnings, though, should probably be a different PR.

edit: I see you added the missing_copy_impls. That's fine in this PR, it looks relatively small impact. I'll open a separate pr for the other settings, we can debate there.

Copy link
Member

@bluejekyll bluejekyll left a comment

Choose a reason for hiding this comment

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

Some questions/concerns to discuss around the potential programming errors on Message and RecordSet...

crates/proto/src/op/message.rs Outdated Show resolved Hide resolved
Name,
RecordType,
DNSClass,
u32,
Copy link
Member

Choose a reason for hiding this comment

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

similar question to that of the Message into_parts function.

crates/proto/src/op/query.rs Show resolved Hide resolved
@leshow
Copy link
Contributor Author

leshow commented Mar 7, 2021

Okay, I've updated this PR with the XParts types. The only one I didn't change was SerialMessage and DnsRequest these both still return tuples, but they only have 2 fields so it sort of makes sense.

For the sake of consistency, I can make SerialMessageParts as well but it would be a breaking change in the API.

Also added From<Message> for MessageParts impls for all the types where this functionality was added.

Copy link
Member

@bluejekyll bluejekyll left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@leshow leshow force-pushed the proto_into_parts branch 3 times, most recently from 6548b6c to 64bd299 Compare March 7, 2021 22:48
@leshow
Copy link
Contributor Author

leshow commented Mar 8, 2021

Sorry for all the pushes, just trying to satisfy the CI here and get a pass

@bluejekyll
Copy link
Member

And it looks like you did. I think This is ready to merge. Do you mind if I squash it into main?

@bluejekyll
Copy link
Member

Oh. Looks like this needs a rebase.

@leshow
Copy link
Contributor Author

leshow commented Mar 8, 2021 via email

@leshow
Copy link
Contributor Author

leshow commented Mar 8, 2021

Should be good to go now 🤞

@bluejekyll bluejekyll merged commit a44d441 into hickory-dns:main Mar 8, 2021
@bluejekyll
Copy link
Member

Thanks for the PR!

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.

None yet

3 participants