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 Debug/Display impl on DateTime #3161

Open
rcoh opened this issue Nov 9, 2023 · 6 comments
Open

Fix Debug/Display impl on DateTime #3161

rcoh opened this issue Nov 9, 2023 · 6 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@rcoh
Copy link
Collaborator

rcoh commented Nov 9, 2023

Currently, DateTime lacks a display impl and the debug impl is basically useless since it logs seconds and subsecond nanos. We should add a Display impl that prints out the date in RFC-3339. This also removes the need for a direct dependency on aws_smithy_types to get basic formatting.

@rcoh rcoh added the good first issue Good for newcomers label Nov 9, 2023
@subhajit20
Copy link

Hey @rcoh I would like to take this,
Can you help out a little that in which .rs file I have to put this impl?

@rcoh
Copy link
Collaborator Author

rcoh commented Nov 9, 2023

@subhajit20
Copy link

thank you so much! the file is here:

https://github.com/awslabs/smithy-rs/blob/32655552c1c1cb825981a8193331c8a2d73ffc9f/rust-runtime/aws-smithy-types/src/date_time/mod.rs#L58

Should I make a function with the name Display inside the DateTime impl?

@rcoh
Copy link
Collaborator Author

rcoh commented Nov 9, 2023

You'll need to add an implementation of the Display trait. Docs here: https://doc.rust-lang.org/rust-by-example/hello/print/print_display.html

rcoh added a commit that referenced this issue Nov 17, 2023
      ## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
It implements Display trait For DateTime

<!--- If it fixes an open issue, please link to the issue here -->
#3161

## Description
<!--- Describe your changes in detail -->
I implemented Display trait for DateTime

## Testing
<!--- Please describe in detail how you tested your changes -->
<!--- Include details of your testing environment, and the tests you ran
to -->

I used this test
```rust
    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);
    }
```

<!--- see how your change affects other areas of the code, etc. -->

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [ ] 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._
rcoh added a commit that referenced this issue Nov 17, 2023
rcoh added a commit that referenced this issue Nov 17, 2023
## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
It implements Display trait For DateTime

<!--- If it fixes an open issue, please link to the issue here -->
#3161

## Description
<!--- Describe your changes in detail -->
I implemented Display trait for DateTime

## Testing
<!--- Please describe in detail how you tested your changes -->
<!--- Include details of your testing environment, and the tests you ran
to -->

I used this test
```rust
    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);
    }
```

<!--- see how your change affects other areas of the code, etc. -->

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [ ] 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._

---------

Co-authored-by: Hakan Vardar <[email protected]>
aws-sdk-rust-ci pushed a commit that referenced this issue Nov 17, 2023
## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
It implements Display trait For DateTime

<!--- If it fixes an open issue, please link to the issue here -->
#3161

## Description
<!--- Describe your changes in detail -->
I implemented Display trait for DateTime

## Testing
<!--- Please describe in detail how you tested your changes -->
<!--- Include details of your testing environment, and the tests you ran
to -->

I used this test
```rust
    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);
    }
```

<!--- see how your change affects other areas of the code, etc. -->

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [ ] 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._

---------

Co-authored-by: Hakan Vardar <[email protected]>
@jdisanti jdisanti added the enhancement New feature or request label Apr 5, 2024
@jdisanti
Copy link
Collaborator

jdisanti commented Apr 5, 2024

Leaving open to enhance the Debug impl. The Display impl has been improved already.

@mjn298
Copy link
Contributor

mjn298 commented Apr 29, 2024

I'll handle Debug! RFC-3339 for Debug as well?

github-merge-queue bot pushed a commit that referenced this issue May 1, 2024
## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
According to #3161 , the Debug output was in a marginally legible
format.

<!--- If it fixes an open issue, please link to the issue here -->
#3161 

## Description
<!--- Describe your changes in detail -->
Since there's no expected difference between `Debug` and `Display` for
date_time, the `fmt::Debug` for `date_time` calls the already
implemented `Display`.

## Testing
<!--- Please describe in detail how you tested your changes -->
I added unit tests, and ran `cargo test` within the `rust-runtime`
directory.
<!--- Include details of your testing environment, and the tests you ran
to -->
Aarch64 mac terminal - `$ cd rust-runtime && cargo test`
<!--- see how your change affects other areas of the code, etc. -->
A single test failed:
`client::waiters::backoff::tests::backoff_with_seeded_jitter` at first.
After rebasing on upstream, the failure went away. Thanks for the fix.



## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [X] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or 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._

---------

Co-authored-by: Mike Nissenbaum <[email protected]>
ogghead pushed a commit to ogghead/smithy-rs that referenced this issue May 5, 2024
<!--- Why is this change required? What problem does it solve? -->
According to smithy-lang#3161 , the Debug output was in a marginally legible
format.

<!--- If it fixes an open issue, please link to the issue here -->

<!--- Describe your changes in detail -->
Since there's no expected difference between `Debug` and `Display` for
date_time, the `fmt::Debug` for `date_time` calls the already
implemented `Display`.

<!--- Please describe in detail how you tested your changes -->
I added unit tests, and ran `cargo test` within the `rust-runtime`
directory.
<!--- Include details of your testing environment, and the tests you ran
to -->
Aarch64 mac terminal - `$ cd rust-runtime && cargo test`
<!--- see how your change affects other areas of the code, etc. -->
A single test failed:
`client::waiters::backoff::tests::backoff_with_seeded_jitter` at first.
After rebasing on upstream, the failure went away. Thanks for the fix.

<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [X] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or 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._

---------

Co-authored-by: Mike Nissenbaum <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants