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

feature: Shorthand encmode/decode functions for predefined encoding options #214

Open
e3b0c442 opened this issue Apr 13, 2020 · 3 comments
Labels
enhancement New feature or request has workaround

Comments

@e3b0c442
Copy link

Is your feature request related to a problem? Please describe.
Seeing a bunch of em, _ := cbor.CTAP2EncOptions().EncMode() is a bit of a code smell (at least to me)

Describe the solution you'd like
Given that these predefined options would ostensibly never error, provide similar predefined encoding/decoding modes that can be used inline à la json.NewEncoder(blah)....

Example:
cbor.CTAP2EncMode().Marshal(&obj)

Describe alternatives you've considered

  • continuing as described above
  • creating my own shorthand function in my library

Additional context
n/a, this is just a suggested API improvement

@e3b0c442
Copy link
Author

I should note I'm happy to submit a PR for this if accepted.

@x448
Copy link
Contributor

x448 commented Apr 13, 2020

@e3b0c442 thanks for opening this issue. It escalated quickly from 1 simple func to a full CTAP2 interface providing encoding & decoding funcs. 🤣

Seeing a bunch of em, _ := cbor.CTAP2EncOptions().EncMode() is a bit of a code smell (at least to me)

More than 1 call to cbor.CTAP2EncOptions().EncMode() isn't needed. For best performance, a mode should be created just once and reused.

If you write a func that is called more than once, that func should reuse a mode rather than call EncMode() itself.

I should note I'm happy to submit a PR for this if accepted.

A CTAP2 interface requires more time & effort than what you initially proposed, so I think @fxamacker will code it (if approved). It would involve both encoding and decoding.

Details

Separation of options and modes allows sanity checks and translation into different data structures (to speed up encoding or decoding) so that overhead shouldn't be repeated.

Ideally, mode(s) should be created during startup and preferably before init() runs. Any helper functions requiring modes would simply reuse the modes created during startup.

I agree the API should make CTAP2 easier. CTAP2 is special case. In part because other predefined options are typically starting points for modification.

@fxamacker please consider exporting a CTAP2 interface that provides these:
Marshal, Unmarshal, NewEncoder, NewDecoder.

  • they should be ready to use before init() runs
  • be callable as cbor.CTAP2.Marshal(v), cbor.CTAP2.Unmarshal(b, &v), etc.

CTAP2 Canonical CBOR should be reviewed to make sure decoding (not just encoding) is in compliance with any extra limitations imposed by CTAP2. Use io.LimitReader, etc. if needed.

@fxamacker fxamacker added this to the v2.3.0 milestone Apr 14, 2020
@fxamacker fxamacker added the enhancement New feature or request label Apr 14, 2020
@fxamacker fxamacker modified the milestones: v2.3.0, v2.4.0 May 29, 2021
@fxamacker fxamacker removed this from the v2.4.0 milestone Dec 28, 2021
@extemporalgenome
Copy link

It escalated quickly from 1 simple func to a full CTAP2 interface providing encoding & decoding funcs.

Would it though?

The OP-proposed cbor.CTAP2EncMode() could essentially just return a ctap2EncMode in the same way that defaultEncMode is used already. It's not a different type (thus not a different interface implementation), but rather just another unexported global variable.

Whether or not the extra globals are warranted is an open question.

This could also be handled with a must style function which can be documented as guaranteed to succeed for any of the bundled EncOptions constructors, i.e. must(cbor.CTAP2EncOptions().EncMode()) would not panic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request has workaround
Projects
None yet
Development

No branches or pull requests

4 participants