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

Reorganize daphne_worker #394

Merged
merged 2 commits into from
Sep 21, 2023
Merged

Reorganize daphne_worker #394

merged 2 commits into from
Sep 21, 2023

Conversation

mendess
Copy link
Collaborator

@mendess mendess commented Sep 20, 2023

Split the role implementations in such a way as to mirror the daphne crate and
move the router out of the root file. In the router module also split the leader
and helper implementations as well as get the test routes of the way.

@mendess mendess self-assigned this Sep 20, 2023
@mendess mendess force-pushed the mendess/refactor-daphne-worker branch 4 times, most recently from 53ae0eb to 4041916 Compare September 20, 2023 18:15
Copy link
Contributor

@cjpatton cjpatton left a comment

Choose a reason for hiding this comment

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

I like it! A few minor suggestions in-line, plus a few asks here:

  1. Please clean up daphne_worker_test/wrangler.toml (remove the admin token)
  2. Add Copyright info to new files
  3. In the future, it would be easier to review these big code movement PRs if it is made up of a bunch of commits, where in each commit, one component (say an impl) is moved at a time.
  4. How about calling the new dap folder roles? This would line up better with the daphne crate.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest "internal_routes.rs"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the name "internal_route" is weird because all routes are public in the sense that they are exposed by the service equally. I think test routes is more evocative of when they are enabled.

daphne_worker/src/router/mod.rs Outdated Show resolved Hide resolved
@mendess mendess force-pushed the mendess/refactor-daphne-worker branch from 4041916 to 76aeea1 Compare September 21, 2023 09:32
@mendess mendess force-pushed the mendess/refactor-daphne-worker branch from 76aeea1 to 1c15598 Compare September 21, 2023 09:38
@mendess mendess merged commit c93e811 into main Sep 21, 2023
4 checks passed
@mendess mendess deleted the mendess/refactor-daphne-worker branch September 21, 2023 10:02
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