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

A few small lints #7

Merged
merged 1 commit into from
Sep 28, 2024
Merged

A few small lints #7

merged 1 commit into from
Sep 28, 2024

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Sep 27, 2024

  • very long identifiers make the code very hard to read. Almost as hard as having tiny identifiers. I think it may make sense to rename a few things to make things more concise.
  • a few clippy suggestion fixes
  • documentation links
  • update all dependencies - there was one DNS record breaking change

Note that you can use #![doc = include_str!("../README.md")] trick in the lib.rs to include README as the crate documentation. This way it the tests in README will be validated, and the content will only be used once.

* very long identifiers make the code very hard to read. Almost as hard as having tiny identifiers.  I think it may make sense to rename a few things to make things more concise.
* a few clippy suggestion fixes
* documentation links

Note that you can use `#![doc = include_str!("../README.md")]` trick in the lib.rs to include README as the crate documentation. This way it the tests in README will be validated, and the content will only be used once.
@thomasarmel thomasarmel merged commit cd9f7af into thomasarmel:master Sep 28, 2024
1 check passed
@thomasarmel
Copy link
Owner

Nice thank you very much!
It looks like it solves issue #6 then...

I'll publish the new version so

@nyurik
Copy link
Contributor Author

nyurik commented Sep 28, 2024

Thanks! What do you think about naming? Would you be OK if I made some name changes to make code more readable?

@thomasarmel
Copy link
Owner

Depends on what you want to change. Could you give some examples?
Personally speaking, I prefer precise over concise naming, so for example, and it's my personal opinion, I think renaming ServerMockerInstructionsList to something like SMInstrList is not a good idea, as it makes the code more confusive. So please tell me what you want to change.

Btw at the time I started this project I was a Rust newbie so I understand there could be some bad practices, I would be grateful for any advice 😄

@thomasarmel
Copy link
Owner

Btw #6 isn't solved but it seems to occur less often 🤔

@nyurik nyurik deleted the linting branch September 29, 2024 01:13
@nyurik
Copy link
Contributor Author

nyurik commented Sep 29, 2024

Heh, not certain of the #6 - I haven't looked at why it might be failing, so I was surprised you said it fixed things.

Naming is hard, I agree. I think HTTP mock server mockito has a fairly well defined, concise API that you might want to mimic.

For example, this I believe is pretty verbose, to the point of actually being unreadable:

mock.add_mock_instructions_list(ServerMockerInstructionsList::new_with_instructions(&[
  ServerMockerInstruction::SendMessageDependingOnLastReceivedMessage(...)
]));

I would get rid of the ServerMockerInstructionsList all together -- you don't need to pass a struct to the mock, you can just give it the commands. At minimum, it would be something like this (work in progress, subject to discussion and ideas):

// Create fake UDP server
let server = FauxUdpServer::with_port(4321).unwrap(); // specific port
let server = FauxUdpServer::with_port_v6(4321).unwrap(); // specific port on IPv6
let server = FauxUdpServer::new().unwrap(); // any port
let server = FauxUdpServer::new_v6().unwrap(); // any port on IPv6

// get server port -- there is only one type of port for a server, keep things simple
let port = server.port();

// the string representation of the server address
let port = server.host_with_port();

// Sometimes users may want the whole socket - easier to just return that
// returns SocketAddr that includes the port. Note that it contains both V4 and V6 variants
let socket = server.socket_address();

// Add mock instructions
server.receive_message();
server.send_message(&[1, 2, 3]);
server.receive_message_smaller_than(1024);
server.custom_responder(|msg| {
    if msg == b"foo" {
        Some(b"bar".to_vec())
    } else {
        None
    }
});
server.stop();

Better yet, if you adapt the mockito API, it could even be something like I described in lipanski/mockito#204 -- perhaps we could merge the two projects?

@thomasarmel
Copy link
Owner

Ok looks an interesting idea!
I have plenty of things to do these days so it won't be my top priority but every contribution is welcomed 😄 .

Do you use this crate for specific needs?

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