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.

Sign up for GitHub

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

Jump to bottom

handle time alias types #1505

Closed
wants to merge 1 commit into from
Closed

handle time alias types #1505

wants to merge 1 commit into from

Conversation

rgarcia
Copy link

@rgarcia rgarcia commented Sep 6, 2017

The dynamodbattribte package handles aliases of builtin types (e.g. aliased strings, ints, etc.), but not time.Time. This PR adds support for aliased time.Time types. I added encode/decode tests that mirror the tests for non-aliased time.Time.

@jasdel jasdel self-requested a review September 12, 2017 17:12
@jasdel jasdel self-assigned this Sep 12, 2017
@jasdel jasdel mentioned this pull request Sep 12, 2017
Merged
@jasdel
Copy link
Contributor

jasdel commented Sep 12, 2017

Thanks for putting this PR together @rgarcia. The change looks good. I made a minor tweak to this PR to cache thereflect.TypeOf(time.Time{}) calls as a minor performance improvement. I'll pull this change in via #1520.

@jasdel jasdel closed this Sep 12, 2017
jasdel added a commit that referenced this pull request Sep 12, 2017
@jasdel
service/dynamodb/dynamodbattribute: Add support for time alias. (#1520)
9b5aaeb 
The `dynamodbattribte` package handles aliases of builtin types (e.g. aliased strings, ints, etc.), but not `time.Time`. This PR adds support for aliased `time.Time` types. I added encode/decode tests that mirror the tests for non-aliased `time.Time`.

Modification of #1505
@awstools awstools mentioned this pull request Sep 13, 2017
Merged
@diehlaws diehlaws added needs-review This issue or pull request needs review from a core team member. and removed review-needed labels Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasdel jasdel Awaiting requested review from jasdel

Assignees

@jasdel jasdel

Labels
needs-review This issue or pull request needs review from a core team member.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rgarcia @jasdel @diehlaws