Skip to content

Making Error type public so that consumers don't have to create their own type alias #298

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

Merged
merged 5 commits into from
Mar 12, 2021

Conversation

coltonweaver
Copy link
Contributor

Issue #, if available: #297

Description of changes: The Error type alias was formerly public only to the lambda_runtime crate. Made this public and replace customer type aliases elsewhere.

By submitting this pull request

  • I confirm that my contribution is made under the terms of the Apache 2.0 license.
  • I confirm that I've made a best effort attempt to update all relevant documentation.

@bahildebrand
Copy link
Contributor

You beat me to the punch by minutes!

@coltonweaver
Copy link
Contributor Author

@bahildebrand My bad I should have marked this one as assigned!

Copy link
Contributor

@bahildebrand bahildebrand left a comment

Choose a reason for hiding this comment

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

LGTM other than the small change.

@@ -29,7 +25,7 @@ struct Response {
}

#[tokio::main]
async fn main() -> Result<(), Error> {
async fn main() -> Result<(), lambda_runtime::Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we include the error type instead? Same for the other examples as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah I totally agree, I think it would look better as an include. Let me address that and push the change up.

@bahildebrand
Copy link
Contributor

@bahildebrand My bad I should have marked this one as assigned!

All good haha! It wasn't a large change, so not much time was spent on it.

Copy link
Contributor

@bahildebrand bahildebrand left a comment

Choose a reason for hiding this comment

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

Sorry I meant to approve on the last one. My comment is a nit. Up to you if you want to address it.

Copy link
Contributor

@rimutaka rimutaka left a comment

Choose a reason for hiding this comment

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

Looks good and timely for v.0.3

@coltonweaver
Copy link
Contributor Author

Sorry I meant to approve on the last one. My comment is a nit. Up to you if you want to address it.

I agree with your comment! Just pushed a commit up with that change.

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.

3 participants