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

Implement Display Trait for DateTime #3183

Merged
merged 4 commits into from
Nov 17, 2023
Merged

Implement Display Trait for DateTime #3183

merged 4 commits into from
Nov 17, 2023

Conversation

HakanVardarr
Copy link
Contributor

Motivation and Context

It implements Display trait For DateTime

#3161

Description

I implemented Display trait for DateTime

Testing

I used this test

    fn test_display_formatting() {
        // Create a DateTime instance for testing
        let datetime = DateTime {
            seconds: 1636761600, // Example timestamp (replace with your actual timestamp)
            subsecond_nanos: 123456789, // Example subsecond nanos (replace with your actual value)
        };

        // Expected RFC-3339 formatted string
        let expected = "2021-11-13T00:00:00.123456789Z";

        // Format the DateTime using Display trait
        let formatted = format!("{}", datetime);

        // Assert that the formatted string matches the expected result
        assert_eq!(formatted, expected);
    }

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

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

@HakanVardarr HakanVardarr requested review from a team as code owners November 12, 2023 09:54
@HakanVardarr
Copy link
Contributor Author

I don't know if I need to any updates on CHANGELOG.next.toml so I didn't do anything with it. If it is necessary can you help me on that?

@rcoh rcoh enabled auto-merge November 12, 2023 11:55
Comment on lines 335 to 338
let datetime =
chrono::NaiveDateTime::from_timestamp_opt(self.seconds, self.subsecond_nanos).unwrap();
let formatted = datetime.format("%Y-%m-%dT%H:%M:%S%.fZ");
write!(f, "{}", formatted)
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of using chrono, we should use the date time support already built into datetime: https://docs.rs/aws-smithy-types/latest/aws_smithy_types/date_time/struct.DateTime.html#method.fmt

make sure to also remove the chrono dependency

Copy link
Collaborator

Choose a reason for hiding this comment

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

we'll also need at least one test

@rcoh rcoh disabled auto-merge November 13, 2023 16:04
@jdisanti
Copy link
Collaborator

I don't know if I need to any updates on CHANGELOG.next.toml so I didn't do anything with it. If it is necessary can you help me on that?

You should add the following:

[[aws-sdk-rust]]
message = "Add `Display` impl for `DateTime`."
references = ["smithy-rs#3183"]
meta = { "breaking" = false, "tada" = true, "bug" = false }
author = "HakanVardarr"

[[smithy-rs]]
message = "Add `Display` impl for `DateTime`."
references = ["smithy-rs#3183"]
meta = { "breaking" = false, "tada" = true, "bug" = false, "target" = "all" }
author = "HakanVardarr"

@HakanVardarr
Copy link
Contributor Author

Is this better?

Copy link
Collaborator

@rcoh rcoh 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!

@rcoh rcoh enabled auto-merge November 14, 2023 20:48
@rcoh rcoh requested a review from a team November 17, 2023 17:53
@rcoh rcoh changed the base branch from main to rollup-merge-11-17 November 17, 2023 18:48
@rcoh rcoh merged commit 8660e39 into smithy-lang:rollup-merge-11-17 Nov 17, 2023
3 checks passed
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.

3 participants