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

MsgpackHandle.RawToString in composite literal no longer works #290

Closed
dcarbone opened this issue Apr 12, 2019 · 5 comments
Closed

MsgpackHandle.RawToString in composite literal no longer works #290

dcarbone opened this issue Apr 12, 2019 · 5 comments

Comments

@dcarbone
Copy link

This commit: a70535d breaks backwards compatibility for anybody who attempted to construct MsgpackHandle as a literal.

Take this example code:

package main

import (
	"fmt"
	"github.com/ugorji/go/codec"
)

func main() {
	fmt.Println(&codec.MsgpackHandle{RawToString: true})
}

Prior to this commit, this worked as expected. Now, however, this no longer works as the location of RawToString has been moved to reside within BasicHandle, which in turns embeds DecodeOptions, which finally has the RawToString field.

This is not the end of the world if you are the direct maintainer of the above code, but when the codec package is only included as a dep of a dep, this becomes a problem.

The inability to properly constrain this repo to 1.1.2 due to package structure only exacerbates this issue.

@ugorji
Copy link
Owner

ugorji commented Apr 12, 2019

Thanks for filing this.

This is limited to

  • users of MsgpackHandle
  • who configure RawToString
  • using composite literals

I can try to re-introduce the RawToString field directly in MsgpackHandle and hack it so that it is kept in sync with the value in DecodeOptions. But I don't want to do that unless there is a clear need for it. If the user base is small and we can get the users to make changes to accommodate, then it may not be worth bastardizing it.

Can you point to the package(s) that uses this, and let me see if they are willing to make changes to their code?

We will also keep this bug open for a bit to see who else clamors for it.

@ugorji ugorji changed the title Moving the location of RawToString breaks API (backward compatibility) Moving RawToString field from MsgpackHandle into embedded DecodeOptions breaks API Apr 12, 2019
@ugorji ugorji changed the title (backward compatibility) Moving RawToString field from MsgpackHandle into embedded DecodeOptions breaks API Moving RawToString field from MsgpackHandle into embedded DecodeOptions breaks API backward compatility Apr 12, 2019
@dcarbone
Copy link
Author

dcarbone commented Apr 12, 2019

I was directly impacted by this while developing a product based on https://github.com/gammazero/nexus.

I have an accepted PR that addresses this: gammazero/nexus#168 but no release has been made.

It was only an issue for me at the time due to the issue I raised in #291. I have since decided that modules is a feature not ready for prime time and have gone back to dep, which does properly determine which version of this package to use based on the govendor file checked in with the nexus repo.

I agree that it is not a huge deal, it was only made an issue for me by the problem presented by #291.

@ugorji
Copy link
Owner

ugorji commented Apr 12, 2019

Ok. Let's leave this open for some weeks and see who else expresses issues with it. We may also be able to get at a more robust solution.

Thanks.

@ugorji ugorji changed the title Moving RawToString field from MsgpackHandle into embedded DecodeOptions breaks API backward compatility MsgpackHandle.RawToString in composite literal no longer works Apr 14, 2019
@dcarbone
Copy link
Author

@ugorji: nexus has since created a release with my fix in place, so this is no longer an issue for myself. Given how few people have responded I'm going to assume this is a very remote edge case, and unless you say otherwise will close this issue later today.

Eager to see what happens with #299.

@ugorji
Copy link
Owner

ugorji commented May 25, 2019

Thanks @dcarbone . I am ok with closing it. Thanks.

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

No branches or pull requests

2 participants