Skip to content

Separate packets in modules #129

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

Merged
merged 4 commits into from
Apr 30, 2025
Merged

Separate packets in modules #129

merged 4 commits into from
Apr 30, 2025

Conversation

andrei-ng
Copy link
Collaborator

@andrei-ng andrei-ng commented Apr 25, 2025

Closes #104, #43, #85 and #33 (I've added a new feature flag for older protocol versions)

@andrei-ng andrei-ng force-pushed the separate-packets-in-modules branch from 5988919 to e4cfe2b Compare April 25, 2025 18:58
@andrei-ng
Copy link
Collaborator Author

andrei-ng commented Apr 25, 2025

@CramBL , without fixing the visibility as done in second commit e4cfe2b I can't get the extraction of a single packet into own file to work.

Making pub(crate) fn validate and pub(crate) const PACKET_SIZE: is not a biggy. But what I don't get is why one needs to add the pub specifier to the tuple struct pub struct #ref_name<'a>(pub(crate) &'a [u8]); to get it to compile when the struct is moved to a different file. What is your expectation?

EDIT: Do you have any ideas for alternatives before proceeding with separating all the other packets?

EDIT2: I guess the answer is inded because of crossing the module boundary as stated here and I don't think there is any other fix except that ...

@andrei-ng andrei-ng force-pushed the separate-packets-in-modules branch 2 times, most recently from fceb3bc to 7ed324e Compare April 25, 2025 20:16
@CramBL
Copy link
Collaborator

CramBL commented Apr 26, 2025

@CramBL , without fixing the visibility as done in second commit e4cfe2b I can't get the extraction of a single packet into own file to work.

Making pub(crate) fn validate and pub(crate) const PACKET_SIZE: is not a biggy. But what I don't get is why one needs to add the pub specifier to the tuple struct pub struct #ref_name<'a>(pub(crate) &'a [u8]); to get it to compile when the struct is moved to a different file. What is your expectation?

EDIT: Do you have any ideas for alternatives before proceeding with separating all the other packets?

EDIT2: I guess the answer is inded because of crossing the module boundary as stated here and I don't think there is any other fix except that ...

I don't really have anything to add, it seems you answered all questions yourself.

considering the massive changes to the macro crate in #124 I would wait with doing macro related refactoring until we land that

@andrei-ng andrei-ng force-pushed the separate-packets-in-modules branch 4 times, most recently from 4173037 to d96b0c4 Compare April 29, 2025 11:45
@andrei-ng
Copy link
Collaborator Author

considering the massive changes to the macro crate in #124 I would wait with doing macro related refactoring until we land that

No worries, I will wait that we first merge in #130 and then change this PR. I only have a few visibility specifier changes in here.

@andrei-ng andrei-ng marked this pull request as ready for review April 29, 2025 11:46
@andrei-ng andrei-ng requested review from CramBL and gwbres April 29, 2025 11:47
@CramBL
Copy link
Collaborator

CramBL commented Apr 29, 2025

considering the massive changes to the macro crate in #124 I would wait with doing macro related refactoring until we land that

No worries, I will wait that we first merge in #130 and then change this PR. I only have a few visibility specifier changes in here.

#130 is merged

@andrei-ng
Copy link
Collaborator Author

#130 is merged

Awesome work @CramBL 🍾

I will check the changes needed for this one asap and push.

@andrei-ng andrei-ng force-pushed the separate-packets-in-modules branch 2 times, most recently from 0475b1d to a9c1eef Compare April 29, 2025 12:56
@andrei-ng
Copy link
Collaborator Author

andrei-ng commented Apr 29, 2025

@CramBL , @gwbres , please review when you get the chance

Copy link
Collaborator

@CramBL CramBL left a comment

Choose a reason for hiding this comment

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

I didn't look much on each individual file, I assume they were mostly just moved around.

I think it's a fantastic refactor, it's a huge improvement to project structure.

@andrei-ng andrei-ng force-pushed the separate-packets-in-modules branch 2 times, most recently from 480e118 to 39bf236 Compare April 29, 2025 17:56
 - introduce proto14 feature flag to close #33

Closes #33, #104, #43, #85

Signed-off-by: Andrei Gherghescu <[email protected]>
Signed-off-by: Andrei Gherghescu <[email protected]>
@andrei-ng andrei-ng force-pushed the separate-packets-in-modules branch from 39bf236 to 72f1b2f Compare April 29, 2025 17:57
@andrei-ng
Copy link
Collaborator Author

I didn't look much on each individual file, I assume they were mostly just moved around.

I think it's a fantastic refactor, it's a huge improvement to project structure.

Yes, 98% just moved around. Only a few minor changes (1 packet renamed) and adding the new proto14 feature flag.

@CramBL
Copy link
Collaborator

CramBL commented Apr 29, 2025

Yes, 98% just moved around. Only a few minor changes (1 packet renamed) and adding the new proto14 feature flag.

I noticed those things, so, great! 🌟

@andrei-ng
Copy link
Collaborator Author

I am merging this so I can continue on the other PRs.

@andrei-ng andrei-ng merged commit d9c8916 into master Apr 30, 2025
13 of 18 checks passed
@andrei-ng andrei-ng deleted the separate-packets-in-modules branch April 30, 2025 06:38
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.

Split packets per module
2 participants