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

feat: Allow specifing how object data is encoded #5139

Merged

Conversation

achingbrain
Copy link
Member

Adds a --data-encoding flag to ipfs 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.

@achingbrain achingbrain requested a review from Kubuxu as a code owner June 19, 2018 11:26
Copy link
Contributor

@schomatis schomatis left a 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.

@achingbrain achingbrain force-pushed the feat/specify-object-data-encoding branch from cac021c to f2375db Compare June 19, 2018 14:33
@achingbrain
Copy link
Member Author

@schomatis done. Do you think it might be worth flipping the default so it outputs base64 by default in the same way that ipfs dag get does?

@schomatis
Copy link
Contributor

Do you think it might be worth flipping the default so it outputs base64 by default in the same way that ipfs dag get does?

Sounds sensible but it would probably break tests that already expect the text as the default format.

@schomatis
Copy link
Contributor

What is up with these Jenkins tests that keep failing for no (apparent) reason?

@achingbrain achingbrain force-pushed the feat/specify-object-data-encoding branch from f2375db to a38055d Compare June 19, 2018 14:50
@achingbrain
Copy link
Member Author

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.

@schomatis
Copy link
Contributor

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 <alex@achingbrain.net>
@achingbrain achingbrain force-pushed the feat/specify-object-data-encoding branch from a38055d to 3cdaea6 Compare June 19, 2018 15:37
@achingbrain
Copy link
Member Author

Fair enough - I've added a couple more tests to cover text encoding and when an invalid encoding is specified.

Copy link
Member

@Stebalien Stebalien left a 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!

@Stebalien Stebalien requested a review from daviddias June 19, 2018 18:19
@Stebalien
Copy link
Member

@diasdavid this modifies the API, you should take a look.

@daviddias
Copy link
Member

daviddias commented Jun 19, 2018

@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 :))

@achingbrain
Copy link
Member Author

@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.

@achingbrain
Copy link
Member Author

Is there anything else outstanding or can this be merged?

@Stebalien Stebalien added the RFM label Jun 30, 2018
@Stebalien
Copy link
Member

This'll make it in after the release (which should happen shortly).

@daviddias
Copy link
Member

@Stebalien, what's the ETA calendar wise? This is blocking some of @achingbrain's work on MFS interop.

@Stebalien
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants