Skip to content

Conversation

@haerdib
Copy link
Contributor

@haerdib haerdib commented Feb 15, 2024

Adds a function new_with_port to each rpc-client which concatenates the address and the port into one string.

closes #109

@haerdib haerdib self-assigned this Feb 15, 2024
@haerdib haerdib added F7-enhancement Enhances an already existing functionality E2-breaksapi labels Feb 15, 2024
@haerdib haerdib requested a review from masapr February 19, 2024 09:01
@haerdib haerdib marked this pull request as ready for review February 19, 2024 09:01
@haerdib haerdib changed the title Pass port and address independently at rpc client creation Rpc-clients: Add creation function with address and port separate Feb 19, 2024
@haerdib haerdib changed the title Rpc-clients: Add creation function with address and port separate Rpc-clients: Add creation function with separate address and port Feb 19, 2024
Copy link
Collaborator

@masapr masapr left a comment

Choose a reason for hiding this comment

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

Added some feedback on testing. But generally good enough this way, so I approve.


let client2 = TungsteniteRpcClient::with_default_url(1);

assert_eq!(client.url, client2.url);
Copy link
Collaborator

Choose a reason for hiding this comment

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

strictly speaking: your testing 2 things, not 1 (and one should ideally test only 1 thing). Apart from testing the new function "with_default_url" you're also testing, that the default port is 9944. Also, what happens if both have an error in their logic? I would check the actual string value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adapted in 9c580cc. Is this what you expected?


let client2 = WsRpcClient::with_default_url();

assert_eq!(client.url, client2.url);
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above

Copy link
Collaborator

@masapr masapr left a comment

Choose a reason for hiding this comment

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

Thanks for the change

@haerdib haerdib merged commit 5ded67a into master Feb 20, 2024
@haerdib haerdib deleted the bh/109-pass-port-independently branch February 20, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

E1-breaksnothing F7-enhancement Enhances an already existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pass adress and port independently to constructor.

3 participants