Skip to content

Conversation

@Jake-Shadle
Copy link
Member

This PR expands on #1296 and implements the actual I/O over a quic stream of subscribing and receiving events.

Ideally #1296 would be merged first so it's easier to review the changes unique to this PR over that one.

@Jake-Shadle
Copy link
Member Author

Hmm, there seems to be a determinism issue in the multiple_subs test, as I've never seen it fail locally, will take a look tomorrow.

@Jake-Shadle
Copy link
Member Author

I haven't yet tracked down the non-determinism bug in the test that was failing yet, so this still shouldn't be merged (or if it is, need to disable that test for now).

Copy link
Member

@XAMPPRocky XAMPPRocky left a comment

Choose a reason for hiding this comment

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

Some code clarity suggestions but looks good to me otherwise.

if qcmp_port.is_none() && icao.is_none() {
return Ok(ExecResponse {
results: vec![ExecResult::Error {
error: "neither THE QCMP port nor ICAO were provided".into(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
error: "neither THE QCMP port nor ICAO were provided".into(),
error: "neither the QCMP port nor ICAO were provided".into(),

rx: reqrx,
};

let task = tokio::task::spawn(async move {
Copy link
Member

Choose a reason for hiding this comment

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

I feel like some of this should be broken up for clarity.

Comment on lines +423 to +427
match response {
proto::Response::V1(res) => {
use proto::v1;

match res {
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be clearer if you collapsed the match statement to a single level.

Suggested change
match response {
proto::Response::V1(res) => {
use proto::v1;
match res {
use proto::v1;
match response {
proto::Response::V1([res](v1::OkResponse::Subscribe(sr))) => {

Comment on lines +218 to +222
match request {
proto::Request::V1(inner) => {
use proto::v1;

match inner {
Copy link
Member

Choose a reason for hiding this comment

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

similar here this match pattern could be collapsed.

Copy link
Member

Choose a reason for hiding this comment

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

I also feel like a lot of the branches here should be factored out into separate functions.

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