-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat: Allow specifing how object data is encoded #5139
feat: Allow specifing how object data is encoded #5139
Conversation
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 -- the test could be moved to test/sharness/t0051-object.sh
.
cac021c
to
f2375db
Compare
@schomatis done. Do you think it might be worth flipping the default so it outputs base64 by default in the same way that |
Sounds sensible but it would probably break tests that already expect the text as the default format. |
What is up with these Jenkins tests that keep failing for no (apparent) reason? |
f2375db
to
a38055d
Compare
Jenkins is happy now, as is Circle but one of the Travis builds failed and I don't seem to have permissions to re-trigger it. |
It's OK, don't worry about it, the tests are actually passing. |
Adds a --data-encoding flag to `ipfs object get` to let the user specify base64 encoding for object data. License: MIT Signed-off-by: Alex Potsides <[email protected]>
a38055d
to
3cdaea6
Compare
Fair enough - I've added a couple more tests to cover text encoding and when an invalid encoding is specified. |
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 is actually a nice, clean, fix. Thanks!
@diasdavid this modifies the API, you should take a look. |
@achingbrain thanks for kicking ass! :D mind triple checking by testing with js-ipfs-api and see if everything works well? (you probably already did it, just quadruple check :)) |
@diasdavid I've been using it locally, it all seems fine. It doesn't change the default output, only adds an option for the data encoding so it's not a breaking change. |
Is there anything else outstanding or can this be merged? |
This'll make it in after the release (which should happen shortly). |
@Stebalien, what's the ETA calendar wise? This is blocking some of @achingbrain's work on MFS interop. |
We're planning on cutting a release today, after which we'll merge this. The upside is that the mplex fixes will be released. The downside is that this'll have to wait a day. |
Adds a
--data-encoding
flag toipfs object get
to let the user specify base64 encoding for object data.I've found that the text encoding used by default can get mangled during JSON serialisation/deserialisation so using base64 instead seems to make it more resilient.
This allows the test in ipfs-inactive/interface-js-ipfs-core#302 to pass.
--
I've not written much go, so hopefully this PR is ok - I couldn't find much in the way of unit tests around the
ipfs object
commands though may just have been looking in the wrong place so have added a sharness test.