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

Add an RBS signature for sigv4 #3152

Merged

Conversation

kazuyainoue0124
Copy link
Contributor

I received some advice suggesting, "Why not start with aws-sigv4?" in this PR: #3150.

Following that advice, I have added type definition files to aws-sigv4.

@mullermp
Copy link
Contributor

mullermp commented Dec 4, 2024

Thank you, we will review this. I think you will also need: a changelog entry, add sig/ to gemspec files, and ensure rbs test passes (see tasks/rbs.rake).

@mullermp
Copy link
Contributor

mullermp commented Dec 4, 2024

@ksss may also be interested in reviewing if you'd like.

Copy link
Contributor

@alextwoods alextwoods left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, this is looking good!

gems/aws-sigv4/sig/error.rbs Outdated Show resolved Hide resolved
gems/aws-sigv4/sig/error.rbs Outdated Show resolved Hide resolved
gems/aws-sigv4/sig/request.rbs Outdated Show resolved Hide resolved
gems/aws-sigv4/sig/signer.rbs Outdated Show resolved Hide resolved
gems/aws-sigv4/sig/signer.rbs Outdated Show resolved Hide resolved
gems/aws-sigv4/sig/signer.rbs Outdated Show resolved Hide resolved
Copy link
Contributor

@ksss ksss left a comment

Choose a reason for hiding this comment

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

  • Private API types should be removed.
  • The intent and syntax are different.
  • Hash arguments should be supported as well as keyword arguments.
  • What about RBS testing?

gems/aws-sigv4/sig/credentials.rbs Outdated Show resolved Hide resolved
gems/aws-sigv4/sig/credentials.rbs Outdated Show resolved Hide resolved
gems/aws-sigv4/sig/request.rbs Outdated Show resolved Hide resolved
gems/aws-sigv4/sig/request.rbs Show resolved Hide resolved
gems/aws-sigv4/sig/request.rbs Outdated Show resolved Hide resolved
gems/aws-sigv4/sig/signer.rbs Outdated Show resolved Hide resolved
gems/aws-sigv4/sig/signer.rbs Show resolved Hide resolved
@kazuyainoue0124
Copy link
Contributor Author

I replaced URI::HTTP | URI::HTTPS with untyped because using URI::HTTP | URI::HTTPS caused an RBS::NoTypeFoundError.

@mullermp
Copy link
Contributor

mullermp commented Dec 9, 2024

Let me know when this is ready for re-review and I can allocate some time.

gems/aws-sigv4/sig/signer.rbs Outdated Show resolved Hide resolved
@kazuyainoue0124
Copy link
Contributor Author

I've made changes based on your feedback. Please review the updated code at your convenience.

Copy link
Contributor

@ksss ksss left a comment

Choose a reason for hiding this comment

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

We should modify tasks/rbs.rake to run the RBS test.

gems/aws-sigv4/sig/signer.rbs Show resolved Hide resolved
@mullermp
Copy link
Contributor

mullermp commented Dec 17, 2024

Is this ready for re-review from our side?

Copy link
Contributor

@mullermp mullermp 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 definitely an improvement!

gems/aws-sigv4/sig/signer.rbs Outdated Show resolved Hide resolved
@mullermp
Copy link
Contributor

I added the sigv4 gem to testing but there's a couple failures. So I think we're really close. I'm not sure how to add Tempfile to rbs - the signature doesn't exist, and then we just need credentials/credential provider interfaces.

@ksss
Copy link
Contributor

ksss commented Jan 9, 2025

To add stdlib (e.g. tempfile) dependencies that are not in the Gemfile.lock, you need to add the manifest.yaml file.

https://dev.to/pocke/rbs-introduced-manifestyaml-2idc

Copy link
Contributor

@alextwoods alextwoods left a comment

Choose a reason for hiding this comment

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

Nice!

@mullermp mullermp merged commit c038125 into aws:version-3 Jan 10, 2025
30 checks passed
@mullermp
Copy link
Contributor

Thank you @kazuyainoue0124

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.

5 participants