Conversation
| public CppWriter writeNamespaceClose(String namespace) { | ||
| write("} // namespace $L", namespace); | ||
| return this; | ||
| } |
There was a problem hiding this comment.
Being a bit nitpicky but Instead of manually opening and closing namespace blocks, why don't we do something like this:
public CppWriter writeNamespaceOpen(String namespace) {
writer.openBlock("namespace $L\n{", namespace);
return this;
}
and the same for writeNamespaceClose, but with writer.closeBlock. Its functionally the exact same (block functions indent/dedent which doesn't matter since we clang format anyway) , but we should try to use these functions when opening/closing blocks
0fd0691 to
661e748
Compare
| implementation("software.amazon.smithy:smithy-aws-traits:1.51.0") | ||
| implementation("software.amazon.smithy:smithy-waiters:1.51.0") | ||
| implementation("software.amazon.smithy:smithy-rules-engine:1.51.0") | ||
| implementation("software.amazon.smithy:smithy-aws-endpoints:1.51.0") |
There was a problem hiding this comment.
Why do we need this? It's not like we're going to be using smithy based endpoints rule traits right?
There was a problem hiding this comment.
Ah, this was from copying the pattern used in our codegen-workshop.
I will refactor the build script to use our existing pattern from smoke-test gen
There was a problem hiding this comment.
Smithy actually needs all the above dependencies, in the current set up we are not accepting any unknown traits
| parser.run(); | ||
| } | ||
|
|
||
| public void render(CppWriter writer) { |
There was a problem hiding this comment.
Why do we render this from scratch here? It may be ok for this particular test but should we have a more generic test file rendering logic that is not specific to Paginators?
There was a problem hiding this comment.
I agree. I can abstract the common parts like header includes, test class structure, and basic compilation test setup into CompilationTestParser as well.
There was a problem hiding this comment.
Refactorted the compilation test to have a BaseCompilationGenerator, if its needed we can extend this class for other test rendering logics
| namespace Aws { | ||
| namespace S3 { | ||
|
|
||
| using ListBucketsPaginator = |
There was a problem hiding this comment.
so lets talk about how we would use this Paginator then. i assume a code example would look something like
const auto s3_client = Aws::MakeShared<S3Client>("alloc-tag");
for (const auto& page: ListBucketsPaginator{s3_client, listReq}) {
//...
}when I look at the java documentation i see that paginator construction is a method on the client. i.e.
ListObjectsV2Iterable listRes = s3.listObjectsV2Paginator(listReq);
for (S3Object content : listRes.contents()) {
//...
}so my question here is why are we doing it differently than java? one of the most common complaints we get is that SDKs have different ergonmics when you switch bettween them.
so why are we creating paginators different than java and not a as a member? what is preventing us from making it a member function?
shouldnt it look something like this?:
S3Client s3_client{};
for (const auto& page: s3_client.ListBucketsPaginator{listReq}) {
//...
}20097ca to
7433789
Compare
…ename reshape priority
…nged shapeutil logic to correctly use the legacy get/set collision rule
Fix pagination traits generator for nested tokens andle explicit nested tokens like "EngineDefaults.Marker" correctly
abbreviation and servicename mismatch
Backward compatibility map for operations that must use "SdkResult" suffix.
S3's ListParts operation has NextPartNumberMarker as integer in C2J but string in Smithy.
…nPlugin to templates
…mber variable tableName
…hods for pagination, and smithy generator to make the base client templates. comment out the integration test. move the model traits into the service model dir instead of pagination dir erated/src/aws-cpp-sdk-bedrock-agentcore-control/include/aws/bedrock-agentcore-control/model/CreateOnlineEvaluationConfigRequest.h
feb8542 to
491f11f
Compare
2e48a84 to
bb8aeb7
Compare
…test adding back removed dependencies and uncommenting dynamo integration test
034cc1a to
a426d02
Compare
a426d02 to
03ae46d
Compare
| namespace Aws { | ||
| namespace DynamoDB { | ||
|
|
||
| using ListContributorInsightsPaginator = Aws::Utils::Pagination::PagePaginator<DynamoDBClient, Model::ListContributorInsightsRequest, |
There was a problem hiding this comment.
lets change PagePaginator to Paginator, unless you feel strongly about the name
|
|
||
| namespace Aws { | ||
| namespace DynamoDB { | ||
| class DynamoDBClient; |
There was a problem hiding this comment.
instead of forward declaring DynamoDB why not have ScanPaginationTraits take a template parameter template <typename Client = ClientType> like you have on Invoke then the entire class is tempalted. You already pass DynamoDBClient as a template parameter in DynamoDBClientPagination.h
| DynamoDBEndpointProviderBase, smithy::client::JsonOutcomeSerializer, | ||
| smithy::client::JsonOutcome, Aws::Client::DynamoDBErrorMarshaller> { | ||
| smithy::client::JsonOutcome, Aws::Client::DynamoDBErrorMarshaller>, | ||
| public DynamoDBPaginationBase<DynamoDBClient> { |
| ListContributorInsightsPaginator(const Model::ListContributorInsightsRequest& request) { | ||
| return Aws::Utils::Pagination::PagePaginator<DerivedClient, Model::ListContributorInsightsRequest, | ||
| Pagination::ListContributorInsightsPaginationTraits>{ | ||
| std::shared_ptr<DerivedClient>(static_cast<DerivedClient*>(this), [](DerivedClient*) {}), request}; |
There was a problem hiding this comment.
likely isnt what we want to be doing, you are creating a shared point from this then have a custom deleter [](DerivedClient*) {} that does nothing when delete is called. you are essentially trying to fit this into a shared pointer without using the actual properties of shared.
consider
#include <iostream>
#include <thread>
template<typename crtp_t>
class thing {
public:
thing(crtp_t& ref) : ptr_(ref) {};
void do_something() {
ptr_.work();
}
private:
crtp_t& ptr_;
};
template <typename crtp_t>
class mixin {
public:
void operation() {
thing<crtp_t> operation_thing{*static_cast<crtp_t*>(this)};
operation_thing.do_something();
}
};
class widget : public mixin<widget> {
public:
void work() {
std::cout << "hello from class\n";
}
};
auto main() -> int {
widget w{};
w.operation();
return 0;
}which is long way of saying on PagePaginator make client a reference not a pointer
|
|
||
| TEST_F(DynamoDBPaginationCompilationTest, DynamoDBPaginationHeadersCompile) | ||
| { | ||
| // Test passes if compilation succeeds |
|
|
||
| class ScanPaginationTest : public Aws::Testing::AwsCppSdkGTestSuite { | ||
| protected: | ||
| std::shared_ptr<DynamoDBClient> dynamoClient; |
There was a problem hiding this comment.
nit: why shared pointer and not stack varable?
| print(f"Code generation done, (re)generated {len(done)} packages.") # Including defaults and partitions | ||
|
|
||
| # Format generated client code | ||
| generated_clients = [service for service in self.c2j_models.keys()] |
There was a problem hiding this comment.
is this not needed in this code path anymore?
| "cloudwatch-logs":"logs", | ||
| "directory-service":"ds", | ||
| "elasticsearch-service ":"es", | ||
| "elasticsearch-service":"es", |
| var model = context.getModel(); | ||
|
|
||
| // Handle legacy services without Smithy models | ||
| if (context.getProjectionName().endsWith(".mock")) { |
There was a problem hiding this comment.
how is ".mock" generated in the projection name?
e7a992b to
239688c
Compare
Issue #, if available:
Description of changes:
Smithy-based code generator for paginators
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.