-
Notifications
You must be signed in to change notification settings - Fork 54
Structure for moving views and params structs out of common #374
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
Changes from all commits
4b4e932
858281e
0914192
818041b
665a9eb
02e0afe
ff05c28
2fc08d4
02d17ac
9da2fd7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,8 @@ use crate::db::schema::{ | |
organization, oximeter, project, rack, router_route, sled, vpc, vpc_router, | ||
vpc_subnet, | ||
}; | ||
use crate::external_api::params; | ||
use crate::internal_api; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this asymmetry is a little janky, but there are so few internal ones that it kind of works. could do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it's that bad. In particular having to be explicit about internal params is definitely good. We really do want to see that from the call site. |
||
use chrono::{DateTime, Utc}; | ||
use db_macros::{Asset, Resource}; | ||
use diesel::backend::{Backend, BinaryRawValue, RawValue}; | ||
|
@@ -343,7 +345,7 @@ pub struct Organization { | |
|
||
impl Organization { | ||
/// Creates a new database Organization object. | ||
pub fn new(params: external::OrganizationCreateParams) -> Self { | ||
pub fn new(params: params::OrganizationCreate) -> Self { | ||
let id = Uuid::new_v4(); | ||
Self { | ||
identity: OrganizationIdentity::new(id, params.identity), | ||
|
@@ -359,12 +361,6 @@ impl DatastoreCollection<Project> for Organization { | |
type CollectionIdColumn = project::dsl::organization_id; | ||
} | ||
|
||
impl Into<external::Organization> for Organization { | ||
fn into(self) -> external::Organization { | ||
external::Organization { identity: self.identity() } | ||
} | ||
} | ||
|
||
/// Describes a set of updates for the [`Organization`] model. | ||
#[derive(AsChangeset)] | ||
#[table_name = "organization"] | ||
|
@@ -374,8 +370,8 @@ pub struct OrganizationUpdate { | |
pub time_modified: DateTime<Utc>, | ||
} | ||
|
||
impl From<external::OrganizationUpdateParams> for OrganizationUpdate { | ||
fn from(params: external::OrganizationUpdateParams) -> Self { | ||
impl From<params::OrganizationUpdate> for OrganizationUpdate { | ||
fn from(params: params::OrganizationUpdate) -> Self { | ||
Self { | ||
name: params.identity.name.map(|n| n.into()), | ||
description: params.identity.description, | ||
|
@@ -396,26 +392,14 @@ pub struct Project { | |
|
||
impl Project { | ||
/// Creates a new database Project object. | ||
pub fn new( | ||
organization_id: Uuid, | ||
params: external::ProjectCreateParams, | ||
) -> Self { | ||
pub fn new(organization_id: Uuid, params: params::ProjectCreate) -> Self { | ||
Self { | ||
identity: ProjectIdentity::new(Uuid::new_v4(), params.identity), | ||
organization_id: organization_id, | ||
} | ||
} | ||
} | ||
|
||
impl Into<external::Project> for Project { | ||
fn into(self) -> external::Project { | ||
external::Project { | ||
identity: self.identity(), | ||
organization_id: self.organization_id, | ||
} | ||
} | ||
} | ||
|
||
/// Describes a set of updates for the [`Project`] model. | ||
#[derive(AsChangeset)] | ||
#[table_name = "project"] | ||
|
@@ -425,8 +409,8 @@ pub struct ProjectUpdate { | |
pub time_modified: DateTime<Utc>, | ||
} | ||
|
||
impl From<external::ProjectUpdateParams> for ProjectUpdate { | ||
fn from(params: external::ProjectUpdateParams) -> Self { | ||
impl From<params::ProjectUpdate> for ProjectUpdate { | ||
fn from(params: params::ProjectUpdate) -> Self { | ||
Self { | ||
name: params.identity.name.map(Name), | ||
description: params.identity.description, | ||
|
@@ -852,7 +836,7 @@ pub struct OximeterInfo { | |
} | ||
|
||
impl OximeterInfo { | ||
pub fn new(info: &internal::nexus::OximeterInfo) -> Self { | ||
pub fn new(info: &internal_api::params::OximeterInfo) -> Self { | ||
let now = Utc::now(); | ||
Self { | ||
id: info.collector_id, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
pub mod http_entrypoints; | ||
pub mod params; | ||
pub mod views; | ||
Comment on lines
+2
to
+3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why pull these apart? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the way I picture it, models know about params (they are the arguments to the model constructor) but not views (views are functions from model to serializable form). So in the models file we can import the params and refer to them as, e.g., There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yeah, the PR description gives a more long-winded version of the picture. I wrote that last, so it's the most polished. |
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.
I think the goal should be to move all this content out of common and into nexus, is that right?
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.
yes. but it's a big, ugly diff