Move find_all to entity_api #15
Conversation
Also starts to consider error handling patterns. Not sure if this is ultimately where we will go as the usage does not feel quite right but will continue to play with it.
jhodapp
added this to the 0.3 - Defining the APIs focused on establishing a session milestone
| use entity_api::error::EntityApiErrorType; | ||
| use entity_api::error::Error as EntityApiError; | ||
| use entity_api::error::EntityApiError; | ||
| use entity_api::error::Error as EntityApiErrorSuper; |
There was a problem hiding this comment.
Let's talk through this when we meet later today. I think you've definitely exposed a part of how this is set up that could use more thought. I'm not sure I love adding the Super since to me that implies inheritance or nesting. But I also don't have a better option in mind off-hand.
There was a problem hiding this comment.
Indeed, I borrowed the Super nomenclature from Rust modules which I think makes this fair. My approach also localizes the name distinction to this module only instead of EntityApiErrorType having "Type" always. From my understanding of Rust convention in the community, adding "Type" to the end of a type is not a common practice. It is in C/C++ land, but not Rust.
| @@ -22,7 +22,9 @@ impl OrganizationController { | |||
| /// --request GET \ | |||
| /// http://localhost:4000/organizations | |||
| pub async fn index(State(app_state): State<AppState>) -> impl IntoResponse { | |||
| let organizations = OrganizationApi::find_all(&app_state).await; | |||
| let organizations = OrganizationApi::find_all(&app_state) | |||
There was a problem hiding this comment.
I think I prefer keeping the unwrap_or_default detail inside of the entity_api context since to me it's an implementation detail and will always be the behavior we're expecting with a find_all. Open to your thoughts on this.
There was a problem hiding this comment.
That's ok, this was only recognizing that unwrap_or(vec![]) vs unwrap_or_default() should return the same thing. But if you think moving any unwrap into the EntityAPI for Organization is even stronger, I'm good with that. Let's take a look at this further together.
| .one(db) | ||
| .await | ||
| .map_err(|err| err.into()) | ||
| Ok(Entity::find_by_id(id).one(db).await?) |
There was a problem hiding this comment.
I like this a lot. I'm curious, though, if we're now only transforming the error up one level in the actual controller? I think we'd need to write a test to determine this.
There was a problem hiding this comment.
I think you figured this out better in your other branch. Wrapping in Ok() is a valid solution because the Result<> is unwrapped from await(), but we need it to be in a Result<> for both the Ok or Err cases.
But having a naked ? operator is always going to be my favorite solution syntactically, so I'm excited to see what you came up with.
|
Closing in favor of #21 |
Description
describe the intent of your changes here
GitHub Issue: [Closes|Fixes|Resolves] #your GitHub issue number here
Changes
Testing Strategy
describe how you or someone else can test and verify the changes
Concerns
describe any concerns that might be worth mentioning or discussing