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 most of the builtin time module #82

Merged
merged 14 commits into from
Jan 1, 2024

Conversation

unexge
Copy link
Contributor

@unexge unexge commented Dec 31, 2023

Partly addresses #72.

With this PR we still fail to pass some OPA tests:

$ cargo test --test opa --features opa-testutil -- time # You need to pass `opa-testutil` for `test.sleep` function
...
OPA TESTSUITE STATUS
    FOLDER                                    PASS FAIL
    time                                    :   28    4

    TOTAL                                   :   28    4

MISSING FUNCTIONS
          FUNCTION                                 FAILURES
       1: time.parse_duration_ns                   1
          TOTAL                                    1

They fail because we can't parse Go's duration (1m32s) and time layout (2006-01-02T15:04:05.999999999Z07:00) at the moment.
I plan to address them in a follow-up PR.

Copy link
Collaborator

@anakrish anakrish left a comment

Choose a reason for hiding this comment

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

LGTM.

We'd need to include licenses as appropriate.

.into())
}

// Adapted from the official Go implementation:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function can be moved to a separate file that lists the original license.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to address it with 56b3c72, but I am not sure if that's sufficient

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe that is good for now.

Later, we might have to declare a THIRD_PARTY_NOTICES file like https://github.com/openenclave/openenclave/blob/master/THIRD_PARTY_NOTICES
or find a way to avoid the code.

@unexge unexge force-pushed the builtin-time-module branch from 754da53 to 56b3c72 Compare December 31, 2023 22:25
@anakrish anakrish merged commit d2eb3ec into microsoft:main Jan 1, 2024
2 checks passed
@unexge unexge deleted the builtin-time-module branch January 1, 2024 11:22
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.

2 participants