-
Notifications
You must be signed in to change notification settings - Fork 42
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 parts of Plane into a plane-common
crate (DIS-2967)
#849
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
client
crateclient
crate (DIS-2967)
client
crate (DIS-2967)plane-common
crate (DIS-2967)
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.
👍 Looks good to me! Reviewed everything up to a33face in 1 minute and 7 seconds
More details
- Looked at
3379
lines of code in102
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_m24uWA9S6sFjgjAv
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
Some small nits about import whitespace, otherwise LGTM!
Fixed, thanks! |
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.
👍 Looks good to me! Incremental review on 164567a in 36 seconds
More details
- Looked at
98
lines of code in6
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_50RrllTWWWkURskQ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
The main
plane
crate contains a client, but depending on it in another project requires bringing in all of Plane, including the openssl dependency. This breaks the client (and other pieces) out into aplane-common
crate with the parts needed to "consume" Plane as a client, and a crate for Plane itself (in particular, without the database and ACME stuff, which bring in other dependencies that can be hard to wrangle).This looks like a huge PR but there is essentially no net new code here; files are just moved around. A few notes:
plane-dynamic
; it was a hack to make tests compile faster but it should be less necessary now that we have broken up Plane into several crates.plane
depends onplane-common
, butplane-common
does NOT depend onplane
, so things likeExponentialBackoff
(used by the client to manage reconnects) go inplane-common
.Todo:
test_get_metrics
andbackend_lifecycle
tests to pass.plane-common
?