Skip to content

Conversation

shaun-cox
Copy link

In order to help demonstrate how to create a SockAddr from an instance of a libc::sockaddr_storage, @Thomasdezeeuw proposed adding an example in #613.

I also took the liberty of addressing a few markdownlint items in the readme and contributing docs... Hope that was okay.

Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

Example LGTM

///
/// # Examples
/// ```
/// # #[allow(dead_code)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure dead-code warnings are disabled by default in documentation tests.

Copy link
Author

Choose a reason for hiding this comment

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

@Darksonn, I'm running cargo test --all-features locally and seeing this (without the allow(dead_code))

---- src/sockaddr.rs - sockaddr::SockAddrStorage::view_as (line 54) stdout ----
error: function `from_sockaddr_storage` is never used
 --> src/sockaddr.rs:60:4
  |
9 | fn from_sockaddr_storage(recv_address: &sockaddr_storage) -> SockAddr {
  |    ^^^^^^^^^^^^^^^^^^^^^
  |
note: the lint level is defined here
 --> src/sockaddr.rs:52:9
  |
1 | #![deny(warnings)]
  |         ^^^^^^^^
  = note: `#[deny(dead_code)]` implied by `#[deny(warnings)]`

error: aborting due to 1 previous error

Couldn't compile the test.

Do I possibly have something misconfigured locally to cause this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, there is a #![doc(test(attr(deny(warnings))))] in socket2. I didn't know about that. It's because of that.

@Thomasdezeeuw
Copy link
Collaborator

Thanks @shaun-cox, I'll sort out the CI another pr.

@Thomasdezeeuw
Copy link
Collaborator

@shaun-cox can you rebase the pr? It should fix the CI issues

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.

3 participants