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

Fix refs local marshalling #2812

Merged
merged 3 commits into from
Jun 10, 2016
Merged

Fix refs local marshalling #2812

merged 3 commits into from
Jun 10, 2016

Conversation

Kubuxu
Copy link
Member

@Kubuxu Kubuxu commented Jun 6, 2016

Part of #2803

License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
@Kubuxu Kubuxu mentioned this pull request Jun 6, 2016
@Kubuxu Kubuxu added the need/review Needs a review label Jun 6, 2016
marshal := func(v interface{}) (io.Reader, error) {
obj, ok := v.(*RefWrapper)
if !ok {
fmt.Println("%#v", v)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should make this a log.Errorf or a log.Warningf

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it isn't needed, it might be a left over from someone debugging something.
It got here when I was rearranging the code.

License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
@whyrusleeping
Copy link
Member

@RichardLitt this changes the API

@whyrusleeping whyrusleeping merged commit 444a8dc into master Jun 10, 2016
@whyrusleeping whyrusleeping deleted the feature/refs-local-api branch June 10, 2016 18:35
@RichardLitt
Copy link
Member

Thanks @whyrusleeping. See ipfs-inactive/http-api-spec#98.

@jbenet
Copy link
Member

jbenet commented Aug 26, 2016

This is a breaking change, right? The summary in the changelog DOES NOT imply it:

  • I understand that this change was necessary to fix a problem, therefore needing to move to ndjson
  • The old behavior may be desired by people who have implemented this. It should be possible to get the exact old behavior from the API with a flag in case bindings dont want to change all their code just yet. This can be considered deprecated and removable in the future.
  • NEVER assume that "because nobody is reporting a problem that nobody is using it". Very wrong, very brittle, very fail. sad doge.

@jbenet
Copy link
Member

jbenet commented Aug 26, 2016

The changelog should say something like:

  • [BREAKING] Fix NDJSON output of ipfs refs local. (@Kubuxu, Fix refs local marshalling #2812)
    Breaking change necessary to correct buggy behavior. For the old output format, use ipfs refs local --some-option.

@jbenet jbenet mentioned this pull request Aug 26, 2016
@Kubuxu
Copy link
Member Author

Kubuxu commented Aug 26, 2016

You can get the old behaviour using text/plain as a type of http-api.
They change is invisible from the CLI.

I shouldn't have disregarded this.

@jbenet jbenet mentioned this pull request Aug 28, 2016
58 tasks
@whyrusleeping
Copy link
Member

Lets change the changelog to say:

  • [BREAKING] Fix NDJSON output of ipfs refs local. (@Kubuxu, Fix refs local marshalling #2812)
    Breaking change necessary to correct buggy behavior. For the old output format, use ipfs refs local --enc=text.

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

Successfully merging this pull request may close these issues.

4 participants