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

Add bindings for custom un/marshal functions #959

Closed
wants to merge 4 commits into from

Conversation

zekroTJA
Copy link
Contributor

@zekroTJA zekroTJA commented Jun 24, 2021

Because large bots need to handle a lot of API requests and responses as well as web socket event messages which all need to go through JSON serialization and deserialization, I thought it might be advantageous if you would be able to swap out the internal JSON implementation with custom ones like bytedance/sonic as an example.

Therefore, I've added two new types in types.go to define the Marshal and Unmarshal functions.

// MarshalFunc encodes an object instance into JSON byte data.
type MarshalFunc func(v interface{}) (data []byte, err error)

// UnmarshalFunc decodes JSON byte data into an object instance.
type UnmarshalFunc func(data []byte, v interface{}) (err error)

As with other preferences, I've added them to the Session struct.

// The function used to encode object instances
// into JSON data
//
// Defaults to json.Marshal
MarshalFunc MarshalFunc

// The function used to decode JSON data into
// object instances
//
// Defaults to json.Unmarshal
UnmarshalFunc UnmarshalFunc

Both values default to the internal encoding/json implementation on initialization by default.

func New(args ...interface{}) (s *Session, err error) {
	// Create an empty Session interface.
	s = &Session{
		// ...
		MarshalFunc:            json.Marshal,
		UnmarshalFunc:          json.Unmarshal,
	}
	// ...

To utilize the defined functions, It was necessary to pass the Session instance to some functions which utilize Marshal and Unmarshal. Some types which are utilizing these functions as well (like TimeStamp), I left untouched to avoid changing the API.

Because I could not get the unit tests to run properly without authentication failures, I've created a quick and dirty example implementation with my fork and sonic as JSON parser. Here you can find the code of it: https://github.com/zekroTJA/discordgo-sonic


I've set this repo as Draft for the moment just to see if the pull request tests succeed with my changes and make further fixes based on that. Also, I want to implement my fork in one of my own Discord Bots which heavily bases on DiscordGo just to see if there are problems occurring during "field test".

Also, it would be really interesting to get some feedback what you think about this implementation and if you think it fits into the project and makes any sense to implement it this way. :)

Edit: I've now used this implementation in combination with pquerna/ffjson without any issues the last couple of days.

Copy link
Collaborator

@FedorLap2006 FedorLap2006 left a comment

Choose a reason for hiding this comment

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

I think that's a great start, but your code has some problems right now.

restapi.go Outdated Show resolved Hide resolved
discord_test.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
restapi.go Outdated Show resolved Hide resolved
@zekroTJA
Copy link
Contributor Author

I think that's a great start, but your code has some problems right now.

Thank you very much for your review and feedback. :)

@zekroTJA zekroTJA marked this pull request as ready for review June 27, 2021 16:10
Comment on lines +3 to +4
github.com/joho/godotenv v1.3.0 h1:Zjp+RcGpHhGlrMbJzXTrZZPrWj+1vfm90La1wgB6Bhc=
github.com/joho/godotenv v1.3.0/go.mod h1:7hK45KPybAkOC6peb+G5yklZfMxEjkZhHbwpqxOKXbg=
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why this was added here.

@FedorLap2006
Copy link
Collaborator

Also note the conflicts.

@FedorLap2006 FedorLap2006 added this to the v0.25.0 milestone Apr 6, 2022
@FedorLap2006 FedorLap2006 added feature Feature implementation feedback Additional feedback is required labels Apr 6, 2022
@FedorLap2006 FedorLap2006 added the high priority Issue or PR with high priority of merge label Apr 14, 2022
@FedorLap2006
Copy link
Collaborator

After a bit of consideration, we've decided that we should do global scoped marshal/unmarshal instead.
The development will be done in the separate PR, so I'm going to close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature implementation feedback Additional feedback is required high priority Issue or PR with high priority of merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants