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(iroh-net): Improve magicsock module visibility #2371

Merged
merged 11 commits into from
Jun 18, 2024

Conversation

flub
Copy link
Contributor

@flub flub commented Jun 17, 2024

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

  • Self-review.
  • [ ] Documentation updates if relevant.
  • [ ] Tests if relevant.
  • All breaking changes documented.

flub added 9 commits June 17, 2024 12:26
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.
@divagant-martian
Copy link
Contributor

@flub
Copy link
Contributor Author

flub commented Jun 17, 2024

Related https://github.com/n0-computer/iroh/pull/1895/files

huh, good catch. I re-added it. How come the compiler does not complain about this?

@divagant-martian divagant-martian changed the title refactor(iroh-net)!: Improve magicsock module visibility refactor(iroh-net): Improve magicsock module visibility Jun 17, 2024
Copy link
Contributor

@divagant-martian divagant-martian left a 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

@divagant-martian divagant-martian changed the base branch from flub/endpoint-no-endpoint to main June 17, 2024 23:50
@divagant-martian
Copy link
Contributor

Since you did not mention a base PR, I'm assuming this was meant to be main. I'll hold on merging so that you can decide whether we need to re-review the whole changes or if this was indeed on top of another branch, etc.

@flub flub changed the base branch from main to flub/endpoint-no-endpoint June 18, 2024 08:30
@dignifiedquire dignifiedquire added this to the v0.19.0 milestone Jun 18, 2024
@flub
Copy link
Contributor Author

flub commented Jun 18, 2024

Since you did not mention a base PR, I'm assuming this was meant to be main. I'll hold on merging so that you can decide whether we need to re-review the whole changes or if this was indeed on top of another branch, etc.

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

Base automatically changed from flub/endpoint-no-endpoint to main June 18, 2024 08:52
@flub flub enabled auto-merge June 18, 2024 08:54
@flub flub added this pull request to the merge queue Jun 18, 2024
Merged via the queue into main with commit 3b0bb51 Jun 18, 2024
25 checks passed
ppodolsky pushed a commit to izihawa/iroh that referenced this pull request Jun 22, 2024
…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.
@flub flub deleted the flub/magicsock-pub-crate branch October 4, 2024 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants