-
Notifications
You must be signed in to change notification settings - Fork 208
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: date parsing #426
fix: date parsing #426
Conversation
Signed-off-by: seajensen <seajensen@gmail.com>
Signed-off-by: seajensen <seajensen@gmail.com>
Signed-off-by: seajensen <seajensen@gmail.com>
@seajensen What format is not working with the JS date? Basic message uses ISO8601, which I think JS supports |
Apologies, I forgot to link the issue. This is in reference to #301. @JamesKEbert might be able to fill in some additional details. |
Yes! I had meant to follow up on the issue. So, from my findings, ISO 8601 dates do not allow spaces in the name, but RFC 3339 that is an extension to ISO 8601 (as I currently understand it) would allow a space. You are correct that we can parse ISO 8601 strings in both React Native and Node (and I assume Electron), however what is being received from ACA-Py isn't strictly ISO 8601, which maybe is something I should raise on that side. |
ISO 3339 is indeed an extension of ISO 8601. Both ISO's specify that you are allowed to use another delimiter if ALL interacting parties know this ( quite odd IMO ). The Basic messaging RFC also gives an example for an ISO 8601 date with a space as delimiter between date and time. My suggestion is for ACA-Py to change it and change it also in the RFC. However, since both RFCs technically allow it, I am not opposed to this fix. |
@@ -1,7 +1,8 @@ | |||
import { Expose, Type } from 'class-transformer' | |||
import { Expose, Transform, Type } from 'class-transformer' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import { Expose, Transform, Type } from 'class-transformer' | |
import { Expose, Transform } from 'class-transformer' |
Signed-off-by: seajensen <seajensen@gmail.com>
Codecov Report
@@ Coverage Diff @@
## main #426 +/- ##
==========================================
- Coverage 86.24% 85.55% -0.69%
==========================================
Files 249 249
Lines 5264 5352 +88
Branches 784 800 +16
==========================================
+ Hits 4540 4579 +39
- Misses 724 773 +49
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@JamesKEbert As I understand it from this thread, ACA-Py does send ISO 8601. https://chat.hyperledger.org/group/os2iqWFM5k2usnrEP?msg=RR5mMYJdxFcYFBLMQ And after doing a bit more research, it seems that JS So ACA-Py is not necessarily the issue here, which makes this PR more an appropriate fix imo. Is basic message the only date that should be in ISO8601 format? No other properties/messages? |
Not quite, Timo. ACA-Py does send ISO860, but is not sending timezone because the default call in Python doesn't send it. So, yes, we are going to fix ACA-Py to include the timezone. However, all recipients of W3C proofs have to handle cases with and without timezones. |
@TimoGlastra it's worth noting @blu3beri's thoughts above:
I'd recommend the Aries RFC changing to not include the space for additional clarity--however, you are correct that this is technically feasible under ISO 8601 according to Berend's findings, since it's defined in the Aries basic messaging RFC. I am not actually immediately sure on other date fields in Aries (I hadn't gotten to it), but I can look into it. |
Ahh I just identified/realized in RFC 74 DIDComm Best Practices that the use of a space instead of "T" is allowed and called out as supported behavior. |
Summary of Changes
Added transform function for parsing dates from multiple formats. Added transform function to basic messaging.
Related Issues
N/A
Pull Request Checklist
Signed-off-by
line (we use the DCO GitHub app to enforce this).scripts/preflight
to run tests and check code style).PR template adapted from the Python attrs project.