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

Transport API and StubTransport Test Implementation #45

Merged
merged 22 commits into from
Dec 11, 2024
Merged

Conversation

neonphog
Copy link
Collaborator

@neonphog neonphog commented Dec 4, 2024

No description provided.

Base automatically changed from url to main December 5, 2024 16:48
@neonphog neonphog changed the title wip stub transport Transport API and StubTransport Test Implementation Dec 7, 2024
@neonphog neonphog requested a review from a team December 7, 2024 04:12
@neonphog neonphog marked this pull request as ready for review December 7, 2024 04:12
Copy link
Member

@ThetaSinner ThetaSinner left a comment

Choose a reason for hiding this comment

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

Looking good! I particularly like the testing with this one and that the traits come with a working and tested implementation so I can see how it all fits together :)

I have left quite a few comments about naming which is quite a pain. They are suggestions only in most cases!

crates/api/src/transport.rs Outdated Show resolved Hide resolved
crates/api/src/transport.rs Outdated Show resolved Hide resolved
crates/api/src/transport.rs Outdated Show resolved Hide resolved
crates/api/src/transport.rs Outdated Show resolved Hide resolved
crates/api/src/transport.rs Outdated Show resolved Hide resolved
crates/core/src/factories.rs Outdated Show resolved Hide resolved
crates/core/src/factories/stub_transport.rs Outdated Show resolved Hide resolved
crates/core/src/factories/stub_transport.rs Outdated Show resolved Hide resolved
crates/core/src/factories/stub_transport/test.rs Outdated Show resolved Hide resolved
crates/core/src/factories/stub_transport/test.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jost-s jost-s left a comment

Choose a reason for hiding this comment

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

It looks good and is also quite dense. I need to come back to this tomorrow. It's not as second nature to me as it is to you. ;-)

Cargo.toml Outdated Show resolved Hide resolved
crates/api/src/transport.rs Outdated Show resolved Hide resolved
crates/api/src/transport.rs Outdated Show resolved Hide resolved
crates/api/src/transport.rs Outdated Show resolved Hide resolved
crates/api/src/transport.rs Outdated Show resolved Hide resolved
crates/api/src/transport.rs Show resolved Hide resolved
crates/api/src/transport.rs Show resolved Hide resolved
crates/core/src/factories/stub_transport.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jost-s jost-s left a comment

Choose a reason for hiding this comment

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

I'm starting to see how this is coming together. Which may sound as if this is getting to a stage where I wanted it to be, but it rather means that I'm beginning to understand the full picture.

Copy link
Member

@ThetaSinner ThetaSinner left a comment

Choose a reason for hiding this comment

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

Very nice, let's go :)

@neonphog neonphog merged commit ec0a6ef into main Dec 11, 2024
5 checks passed
@neonphog neonphog deleted the transport branch December 11, 2024 19:55
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