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

[MP_EXT] Extension encoder and decoder functions are not exportable #420

Open
rybakit opened this issue Dec 16, 2024 · 2 comments
Open

[MP_EXT] Extension encoder and decoder functions are not exportable #420

rybakit opened this issue Dec 16, 2024 · 2 comments

Comments

@rybakit
Copy link

rybakit commented Dec 16, 2024

Consider making the current MessagePack extension encoder and decoder functions exportable. I’m personally interested in the decimal extension, where I would like to register my own extension but still be able to reuse the encoding logic from the tarantool/decimal package. The easiest way is to simply make the decimalEncoder() and decimalDecoder() functions start with a capital letter. Another option is to make them adhere to the MarshalerUnmarshaler interface from vmihailenco/msgpack/v5 and rename them to MarshalMsgpack() and UnmarshalMsgpack(), respectively.

@rybakit rybakit changed the title [MP_EXT] Extension encoders and decoders are not exportable [MP_EXT] Extension encoder and decoder functions are not exportable Dec 16, 2024
rybakit added a commit to rybakit/go-tarantool that referenced this issue Dec 16, 2024
rybakit added a commit to rybakit/go-tarantool that referenced this issue Dec 16, 2024
rybakit added a commit to rybakit/go-tarantool that referenced this issue Dec 16, 2024
rybakit added a commit to rybakit/go-tarantool that referenced this issue Dec 16, 2024
rybakit added a commit to rybakit/go-tarantool that referenced this issue Dec 16, 2024
@rybakit
Copy link
Author

rybakit commented Dec 25, 2024

@oleg-jukovec, Looking deeper into the decimal extension implementation, it’s not entirely clear why the connector uses a wrapper for the decimal.Decimal type from shopspring/decimal. This made sense in the earlier implementation, where the wrapper provided additional methods like MarshalMsgpack() and UnmarshalMsgpack(). However, after refactoring the code to use msgpack v5, those methods are no longer present, and the wrapper doesn’t appear to serve any useful purpose.

To give some context to the problem I’m trying to solve: I want to register an encoder and decoder directly on the decimal.Decimal type without needing to wrap decimals in the connector's Decimal struct every time.

As an alternative to making decoders and encoders exportable, what do you think about removing the wrapper altogether? This solution would address my use case and align with the approach used for the UUID extension, which doesn’t rely on a wrapper and allows direct encoding and decoding of UUID values in the msgpack extension.

@oleg-jukovec
Copy link
Collaborator

oleg-jukovec commented Dec 29, 2024

I don't like the idea to use a types from external libraries as is into the connector: they may be incompatibility in the future or we can find a better type.

If the library declares its own types with transform methods, then in the future we can add methods with a new types or declare the old methods deprecated.

That is, it is not entirely convenient for the user of the library code, but it is good for the library and its development in the future, backward compatibility etc.

The uuid is an exception because we don't have much time to rewrite it (and it's probably not the best idea to rewrite something that works).

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

No branches or pull requests

2 participants