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

Refactor OsInputOutput (combine interfaces & frames into single Vec) #310

Merged
merged 3 commits into from
Oct 21, 2023

Conversation

cyqsimon
Copy link
Collaborator

@cyqsimon cyqsimon commented Oct 21, 2023

Synopsis

OSInputOutput definition changed from:

pub struct OsInputOutput {
    pub network_interfaces: Vec<NetworkInterface>,
    pub network_frames: Vec<Box<dyn DataLinkReceiver>>,
    pub get_open_sockets: fn() -> OpenSockets,
    pub terminal_events: Box<dyn Iterator<Item = Event> + Send>,
    pub dns_client: Option<dns::Client>,
    pub write_to_stdout: Box<dyn FnMut(String) + Send>,
}

to:

pub struct OsInputOutput {
    pub interfaces_with_frames: Vec<(NetworkInterface, Box<dyn DataLinkReceiver>)>,
    pub get_open_sockets: fn() -> OpenSockets,
    pub terminal_events: Box<dyn Iterator<Item = Event> + Send>,
    pub dns_client: Option<dns::Client>,
    pub write_to_stdout: Box<dyn FnMut(String) + Send>,
}

This is to provide stronger semantic safeguards against accidental & unnoticed screwups.


Unresolved issues

  • This refactor seems to have changed test outputs substantively, when it really shouldn't have changed anything. This needs to be investigated before merge. Okay clearly not. That seems to be due to another commit on my local branch. This is good to merge then, seeing that the test failures are no worse than they were.

@cyqsimon cyqsimon merged commit 6fa77d2 into main Oct 21, 2023
8 of 10 checks passed
@cyqsimon cyqsimon deleted the os-input-output-refactor branch October 21, 2023 14:15
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.

1 participant