-
Notifications
You must be signed in to change notification settings - Fork 165
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(iroh-net): Improve magicsock module visibility #2371
Conversation
Also for EndpointType -> DirectAddressType
This changes the public api because for some reason config was public.
This is more in line with other Addrs, e.g. SocketAddr.
This audits the public visibility of items in the magicsock module. pub(super) and pub(crate) mean the same thing here, since it is a top-level module. This prefers pub(crate) to make it clear who can see the items. Likewise it marks sub-items pub(crate) as well to make the visibility clear. Some items did not need to be visible outside of the module and have been removed. Unfortunately for ControlMsg this is technically a breaking API change, even though there was no practical use for it, it only ever was pub by accident.
huh, good catch. I re-added it. How come the compiler does not complain about this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I updated the PR tittle and description to remove the breaking change. I'm honestly not sure why the compiler is not complaining, or at least I can't remember right now. We should probably check what's going on, but for the scope of the PR that's not an issue
Since you did not mention a base PR, I'm assuming this was meant to be |
The notes section did mention it was on top of the other PR. Setting it to bade directly means it'll include the commits of the other branch as well. Leaving it to that branch means it will automatically rebase on any other commits added to the base branch. I chose this because I suspected doing the two PRs independently would give merge conflicts in one of them, so thought it was easier to sequence them. admittedly github could improve that workflow |
…2371) ## Description This audits the public visibility of items in the magicsock module. pub(super) and pub(crate) mean the same thing here, since it is a top-level module. This prefers pub(crate) to make it clear who can see the items. Likewise it marks sub-items pub(crate) as well to make the visibility clear. ## Breaking Changes ## Notes & open questions This is targetted at n0-computer#2369 which will need to be merged first. ## Change checklist - [x] Self-review. - ~~[ ] Documentation updates if relevant.~~ - ~~[ ] Tests if relevant.~~ - [x] All breaking changes documented.
Description
This audits the public visibility of items in the magicsock module. pub(super) and pub(crate) mean the same thing here, since it is a top-level module. This prefers pub(crate) to make it clear who can see the items. Likewise it marks sub-items pub(crate) as well to make the visibility clear.
Breaking Changes
Notes & open questions
This is targetted at #2369 which will need to be merged first.
Change checklist
[ ] Documentation updates if relevant.[ ] Tests if relevant.