clarkie / dynogels

DynamoDB data mapper for node.js. Originally forked from https://github.com/ryanfitz/vogels
Other
490 stars 110 forks source link

Provide an API for getting the AWS Dynamo CreateTable parameters #144

Closed aneilbaboo closed 6 years ago

aneilbaboo commented 6 years ago

This is a small change which extracts the functionality for generating the Dynamo CreateTable API arguments from Table.prototype.createTable to Table.prototype.dynamoParameters.

I use Serverless to manage deployment (and teardown) of DynamoDB tables. To do this, I need the AWS CreateTable params. I'd prefer to avoid having to keep the Serverless table descriptions in sync with Dynogels, and it's nicer to describe the tables in Dynogels anyway. This change enables that. It's probably useful in some other contexts too.

The new API is:

MyModel.dynamoParameters(options);

Where options are the same as the first argument to Table.prototype.createTable.

Note:

Changed the method from makeCreateTableParams to dynamoParameters.

cdhowie commented 6 years ago

I support this change. Would you mind adding some unit tests for this new method?

aneilbaboo commented 6 years ago

Will do.

aneilbaboo commented 6 years ago

Hi @cdhowie

I've got 4 new commits:

I'm not sure how you feel about that third commit. I can roll it back if you'd like to keep the redundant createTable tests.

aneilbaboo commented 6 years ago

The test is failing because of an unrelated error at test/integration/integration-test.js:544:23. Same problem on master.

aneilbaboo commented 6 years ago

@cdhowie - I made a final tweak - renamed the API to dynamoParameters, which I think reads nicer.

aneilbaboo commented 6 years ago

Also updated the README.

cdhowie commented 6 years ago

@aneilbaboo I think the prior name makes more sense. The new name doesn't make it clear what the parameters are used for.

aneilbaboo commented 6 years ago

@cdhowie How about dynamoCreateTableParameters? That makes it clear what service & what purpose it's for. makeTableCreateParameters was looking a bit confusing in my code.

aneilbaboo commented 6 years ago

@cdhowie - I pushed a commit that changes it to model.dynamoCreateTableParams() - is that ok? Or do you want me to revert to model.makeCreateTableParams()?

cdhowie commented 6 years ago

@aneilbaboo The only issue I see remaining is the presence of the yarn-error.log file. Can you remove that?

aneilbaboo commented 6 years ago

Oooof. Sorry about that.

aneilbaboo commented 6 years ago

Thanks, @cdhowie - Removed it and added yarn-error.log to .gitignore