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.

Sign up for GitHub

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

Jump to bottom

Message ID can be int, string, or null as per OpenRPC spec #48

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Message ID can be int, string, or null as per OpenRPC spec #48

wants to merge 4 commits into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Apr 8, 2021

Fixes: #28

@rvagg rvagg mentioned this pull request Apr 8, 2021
Open
mvdan
mvdan approved these changes Apr 8, 2021
View reviewed changes
handler.go
return nil
}

func (r requestID) MarshalJSON() ([]byte, error) {
Copy link

Choose a reason for hiding this comment

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

You could add a sanity check here that "actual" is one of the three allowed Go types, just as an extra sanity check since UarshalJSON does it too.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks @mvdan, added and it picked up a bug in my code! can you sanity check my latest commit to see if it's how you would have done it?

Copy link

@mvdan mvdan Apr 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

SGTM. You could simplify it a little bit, since you don't need to separate the json.Marshal calls:

	switch r.actual.(type) {
	case nil, int64, string:
		return json.Marshal(r.actual)
	default:
		return nil, fmt.Errorf("unexpected ID type: %T", r.actual)
	}

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 thanks & done

@rvagg rvagg mentioned this pull request Apr 9, 2021
Merged
@rvagg
Copy link
Member Author

rvagg commented Apr 9, 2021

Updated so all the map[uint64]s are map[interface{}] in websockets.go now, I'd left out chanHandlers. I think this is safe because it's per-client so it's therefore up to the client to get the IDs sorted out properly. A test for this via websockets might be nice but I suspect that'd be an enormous amount of work just to test strings.

@rvagg
Copy link
Member Author

rvagg commented Jun 24, 2021

@magik6k any chance of getting some eyes on this? I know it's not a trivial change but it'd be nice to be able to support OpenRPC properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mvdan mvdan mvdan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot parse non-numerical ID
2 participants
@rvagg @mvdan