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 binary type representation #297
Fix binary type representation #297
Conversation
10172ec
to
aa36926
Compare
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. |
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? |
aa36926
to
3c8601c
Compare
Hi, Reduced the size of the MR to have only the fix to binary type representation |
3c8601c
to
1ebca46
Compare
Hi, Rebased after the recent improvements to the UT (good work @JoseIgnacioTamayo !). |
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.
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.
1ebca46
to
3ffe88e
Compare
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
binary
built-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.