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 notes datetime ISO 8601 format #827

Merged
merged 4 commits into from
Aug 12, 2024

Conversation

rodolfomiranda
Copy link
Contributor

@rodolfomiranda rodolfomiranda commented Jul 27, 2024

Notes that are not initiated with a dt parameter, are being created with a default dt that don't follows the ISO 8601 format. The class use the function datetime.datetime.now().isoformat() to create the string timestamp that result in a value without timezone or offset such as 2023-10-05T14:48:23.123456.
Instead, keripy normal use helping.nowIso8601() to create the timestamp that result in values like 2021-06-27T21:26:21.233257+00:00

tests/app/test_notifying.py Outdated Show resolved Hide resolved
@m00sey
Copy link
Member

m00sey commented Jul 31, 2024

Any interest in fixing the other usages of datetime vs helping.nowISO() @rodolfomiranda 😬

@rodolfomiranda
Copy link
Contributor Author

Any interest in fixing the other usages of datetime vs helping.nowISO()

I could only find datetime.datetime used in signaling.py. I updated it. The rest of keripy seems to be ok since datetime is only used for timedelta.

@m00sey
Copy link
Member

m00sey commented Jul 31, 2024

Any interest in fixing the other usages of datetime vs helping.nowISO()

I could only find datetime.datetime used in signaling.py. I updated it. The rest of keripy seems to be ok since datetime is only used for timedelta.

Thanks, appreciate you taking the extra time.

@pfeairheller
Copy link
Member

Can we please explain in the description of the PR why this change is being made? The helping functions are used in many other places in KERIpy, specifically for time in KERI events and credentials. Why are you proposing we change them for only this small subset of date usages?

If the date functions are wrong, why not fix them?

@rodolfomiranda
Copy link
Contributor Author

rodolfomiranda commented Aug 12, 2024

If the date functions are wrong, why not fix them?

date functions are correct, but were not applied correctly in a couple of lines in notifying.py and signaling.py

Copy link
Member

@pfeairheller pfeairheller left a comment

Choose a reason for hiding this comment

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

Wow, I definitely reviewed this PR too early in the morning... I read it backwards!!

Looks good to me!

@pfeairheller pfeairheller merged commit ee5177c into WebOfTrust:main Aug 12, 2024
6 checks passed
@SmithSamuelM
Copy link
Collaborator

Should be using helping.nowIso8601() or helping.toIso8601 everywhere there is a datetime string (non CESR encoded). Or Dater instance if want CESR encoded datetime

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.

4 participants