Fix binary type representation#297
Merged
xavier-contreras merged 1 commit intorobshakir:masterfrom Jul 27, 2023
Merged
Conversation
fperrin
force-pushed
the
fred/fix-binary-type-representation
branch
from
10172ec to
aa36926
Compare
Collaborator
|
Hi Frederic Thanks for your contribution! Those unit tests needed a fix, by chance I also started taking a look last weekend. I would suggest we keep this PR exclusively to fixing the tests, not changing Python versions. We are in the process of setting up GitHub Actions, which will take care of running the tests in several python versions. |
Collaborator
|
After a look at the changes files, I think we need to split it:
Could you please reduce the scope of the PR to proposing changes to the way Binary values are treated? |
fperrin
force-pushed
the
fred/fix-binary-type-representation
branch
from
aa36926 to
3c8601c
Compare
fperrin
changed the title
Contributor
Author
|
Hi, Reduced the size of the MR to have only the fix to binary type representation |
fperrin
force-pushed
the
fred/fix-binary-type-representation
branch
from
3c8601c to
1ebca46
Compare
Contributor
Author
|
Hi, Rebased after the recent improvements to the UT (good work @JoseIgnacioTamayo !). |
Collaborator
xavier-contreras
left a comment
xavier-contreras
left a comment
There was a problem hiding this comment.
Thanks! Just a couple of minor comments.
As described in RFC7959, section 9.8.2, values of the ``binary`` built-in type are represented with the base64 encoding scheme. Encode / decode from that scheme on serialise/deserialise, and use native ``bytes`` type for Python representation.
fperrin
force-pushed
the
fred/fix-binary-type-representation
branch
from
1ebca46 to
3ffe88e
Compare
xavier-contreras
approved these changes
Jul 25, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi @xavier-contreras ; @JoseIgnacioTamayo
It's fantastic you've picked up maintenance for pyangbind! Thank you for stepping up. Now that pyangbind has a new lease of life, I'll submit a series of fixes as and when I clean them up for public consumption. If I can submit those changes in a way that make it easier for you to review, please tell me.
This first set included here fixes representation of the
binarybuilt-in type. It also fixes the unit-tests by pinning the version of YANG models to a known passing version. (In fact, my opinion is that the YANG models we use should be committed to the repo, instead of fetching them over the network on every run. I can make that change if you agree.) The UT pass on python3.9. I removed python2.7 as I'm pretty sure my usage ofsuper()won't work there; I didn't actually run tox with python3.4 to 3.7If there is interest, I can add GitHub actions to run the existing unit-tests on every PR.