Deprecate encoder in signining #523
Conversation
|
@reaperhulk I think it makes sense, if we are willing to "just deal with bytes" in the long term, to just begin deprecating all the encoder= APIs, instead of doing a step just deprecating non-raw encoders. |
|
@dstufft , @reaperhulk , @pyca/pynacl-core: could someone take a look at this to push it forward? |
|
@dstufft @reaperhulk @pyca/pynacl-core ping |
src/nacl/signing.py
Outdated
| @@ -101,13 +116,27 @@ def verify(self, smessage, signature=None, encoder=encoding.RawEncoder): | |||
| signature. | |||
| :rtype: :class:`bytes` | |||
| """ | |||
| if len(args) == 1: | |||
There was a problem hiding this comment.
Same comment about the approach.
src/nacl/signing.py
Outdated
| def __init__(self, seed, encoder=encoding.RawEncoder): | ||
| def __init__(self, seed, *args, **kwargs): | ||
|
|
||
| if len(args) == 1: |
tests/test_signing.py
Outdated
| @pytest.mark.parametrize("hseed", [ | ||
| b"77076d0a7318a57d3c16c17251b26645df4c2f87ebc0992ab177fba51db92c2a", | ||
| ]) | ||
| def test_raising_on_excess_encoder_parameter(self, hseed): |
There was a problem hiding this comment.
These tests aren't really testing anything interesting with the new approach, correct?
There was a problem hiding this comment.
Right, they were redundant. Reverted the relevant commit.
Since, after a75cb45, those test are redundant.
lmctv
changed the title
|
@reaperhulk I hope this is now ready for another review. |
|
@reaperhulk ping. |
|
@reaperhulk Would you mind another review cycle and possibly merge this? I'd rather avoid preparing the |
|
@reaperhulk could you please take a look here and at the proposed plan for a quick release I outlined in #532 (comment) |
| if encoder is None: | ||
| encoder = encoding.RawEncoder | ||
| else: | ||
| warn(("Implicit encoding/decoding of signing keys " |
There was a problem hiding this comment.
We should just say encoding/decoding of signing keys is deprecated. Remove the words explicit and implicit from this msg.
| if encoder is None: | ||
| encoder = encoding.RawEncoder | ||
| else: | ||
| warn(("Automatic encoding/decoding of signed messages " |
There was a problem hiding this comment.
This should be the same message (or better yet, this should be a function that everything invokes)
| if signature is not None: | ||
| # If we were given the message and signature separately, combine | ||
| # them. | ||
| smessage = signature + encoder.decode(smessage) |
There was a problem hiding this comment.
Can we quickly pull this revert into its own PR so we can land that?
There was a problem hiding this comment.
I just reopened the Revert PR #505, and rebased it on current master.
| if encoder is None: | ||
| encoder = encoding.RawEncoder | ||
| else: | ||
| warn(("Implicit encoding/decoding of signing keys " |
| if encoder is None: | ||
| encoder = encoding.RawEncoder | ||
| else: | ||
| warn(("Automatic encoding/decoding of signed messages " |
|
We should still deprecate encoder but we can open a PR for it at some point. |
Revert #504 and deprecate usage of the encoder= parameter in high level signing APIs, as suggested by @reaperhulk in his review of #507.
If we decide to follow this route, I think we should do the same for the other high level APIs too.
Since this is a potentially disruptive change for downstream users, I'd be grateful if also @dstufft could give his opinion on these changes.