- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 59
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
Feature/jer #187
Feature/jer #187
Conversation
Thank you for your PR! I have two initial bits of feedback, the first is that I don't think this should be a feature. If we want to go for that approach we should do that separately and apply it uniformly to the other codecs. The second is the use of serde. Since we don't benefit from the generic codec framework of serde, since we are a codec framework ourselves, wouldn't we be better off using a purpose built json library, like jzon? |
|
@XAMPPRocky I replaced |
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 to me mostly, just a couple comments.
_t: crate::Tag, | ||
_c: Constraints, | ||
) -> Result<TeletexString, Self::Error> { | ||
todo!() |
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.
Why todo here?
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.
Currently we do not have any logic to convert between String
and TeletexString
or to validate a TeletexString
's bytes and it doesn't seem to be a trivial addition to make. PER also has a todo!()
in its Decoder
impl, so I'd propose to tackle this issue in a separate PR.
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.
Just a note when out of curiosity looking this PR, Teletex standard has been withdrawn. Should we just delete the type? https://www.itu.int/rec/T-REC-T.61 https://en.wikipedia.org/wiki/ITU_T.61 and focus on other more used ones. Get back to it if someone really needs it.
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.
Unfortunately it keeps sticking around 🥲 It is also used in rasn-pkix
. However, I think we can afford to be lenient when it comes to the implementations in jer
, xer
, oer
, or per
, since AFAIK T.61 had already been revoked when they were conceived.
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.
Oh, I missed that one. It is mentioned in oer
as (TeletexString, T61String) both, but I haven't faced any standard which actually uses the types yet (when explicitly using OER).
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.
This might be good reference if we end up implementing it:
_c: crate::types::Constraints, | ||
_value: &crate::types::TeletexString, | ||
) -> Result<Self::Ok, Self::Error> { | ||
todo!() |
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.
Same as above
Thank you for your PR! |
Adds support for JSON encoding rules
JER-support is implemented as a non-default feature, both in
rasn
as the featurejer
, as well as inrasn-derive
as the featuretext-based-encodings
(changes inrasn-derive
will likely be needed for XER handling, too). Encoder and decoder are built uponserde_json
.Closes #188
#109