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

fix(data-classes): include milliseconds in scalar types #504

Merged
merged 2 commits into from
Jul 6, 2021

Conversation

michaelbrewer
Copy link
Contributor

@michaelbrewer michaelbrewer commented Jul 2, 2021

Issue #, if available:

#503

Description of changes:

Set the formatted output to include milliseconds

fixes #503

Checklist

Breaking change checklist

Potentially a breaking change if expecting only seconds.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 2, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2021

Codecov Report

Merging #504 (b08802e) into develop (cc128ad) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #504   +/-   ##
========================================
  Coverage    99.90%   99.90%           
========================================
  Files          105      105           
  Lines         4234     4234           
  Branches       206      206           
========================================
  Hits          4230     4230           
  Misses           1        1           
  Partials         3        3           
Impacted Files Coverage Δ
...ilities/data_classes/appsync/scalar_types_utils.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc128ad...b08802e. Read the comment docs.

@michaelbrewer
Copy link
Contributor Author

@heitorlessa I think that %f might cause issues with AppSync dates as they are expecting milliseconds.

@michaelbrewer
Copy link
Contributor Author

@heitorlessa seems to work as microseconds, so should i still trim off the extra 3 positions? or leave it

@michaelbrewer michaelbrewer changed the title fix(dataclasses): include milliseconds in scalar types fix(data-classes): include milliseconds in scalar types Jul 5, 2021
@heitorlessa heitorlessa added the bug Something isn't working label Jul 5, 2021
@heitorlessa
Copy link
Contributor

thanks for the fix!! Let's trim off to stay as milliseconds ([:-3]) to stay compliant whether validation catches server-side - Depending on how much data one pumps in, this should also save on costs

Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

Trim last three to stick as ms [:-3]

@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 5, 2021
Copy link
Contributor

@heitorlessa heitorlessa 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 changes, Mike!

Note to self: break down test_data_classes as it's 1.3K LOC atm

@heitorlessa heitorlessa merged commit 2473480 into aws-powertools:develop Jul 6, 2021
@michaelbrewer michaelbrewer deleted the fix-scalar-types-times branch July 6, 2021 06:20
@michaelbrewer
Copy link
Contributor Author

Thanks for the changes, Mike!

Note to self: break down test_data_classes as it's 1.3K LOC atm

It would be great if pytest can run parts in parallel :). Then i would definitely help split up things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: aws scalar type for AWSDateTime should include milliseconds
3 participants