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

Missing "origin" field added to RespSendJoin #144

Merged
merged 5 commits into from Oct 12, 2019

Conversation

@behouba
Copy link
Contributor

commented Oct 1, 2019

In this PR, i have added the missing "origin" field of RespSendJoin and also made some changes to TestRespSendJoinMarshalJSON.

Copy link
Collaborator

left a comment

Well actually I think it's now worth it to flatten the types i.e. don't type RespSendJoin RespState anymore. For one thing it'll be easier to read, and for the other the MarshalJSON trick to add the 200 is expected to be gone in the future: matrix-org/matrix-doc#1802

@behouba

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2019

Well actually I think it's now worth it to flatten the types i.e. don't type RespSendJoin RespState anymore. For one thing it'll be easier to read, and for the other the MarshalJSON trick to add the 200 is expected to be gone in the future: matrix-org/matrix-doc#1802

So, RespSendJoin will be something like:

type RespSendJoin struct {
        StateEvents []Event `json:"state"`
	AuthEvents  []Event `json:"auth_chain"`
	Origin      string  `json:"origin"`
}
@Cnly

This comment has been minimized.

Copy link
Collaborator

commented Oct 1, 2019

@behouba Yes, I think that would do it.

@behouba

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2019

I have made RespSendJoin it own struct that do no more have it own implemantion of json.Unmarshaler and json.Marshaler.

I have also found type casting between RespSendJoin and RespState in dendrite, so this change wil break dendrite.
@Cnly What do you think about it ?

@behouba behouba requested a review from Cnly Oct 2, 2019
Copy link
Collaborator

left a comment

Hmm, I think an alternative solution to the current situation is a func (r RespSendJoin) ToRespState() RespState so it breaks fewer things.

federationtypes.go Outdated Show resolved Hide resolved
federationtypes.go Show resolved Hide resolved
@behouba

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2019

@Cnly has you suggested i have added a method to func (r RespSendJoin) ToRespState() RespState to type RespSendJoin. I also found this alternative much better.
Now we should remplace each type casting from RespSendJoin to RespState with a call to ToResState() on the given RespSendJoin.

@behouba behouba requested a review from Cnly Oct 4, 2019
Copy link
Collaborator

left a comment

I think we're almost there! :)

federationtypes.go Outdated Show resolved Hide resolved
@Cnly
Cnly approved these changes Oct 9, 2019
Copy link
Collaborator

left a comment

@behouba lgtm! Just nitpicking now.

federationtypes.go Outdated Show resolved Hide resolved
@behouba behouba requested a review from Cnly Oct 10, 2019
@Cnly
Cnly approved these changes Oct 12, 2019
@Cnly Cnly merged commit 2ab3063 into matrix-org:master Oct 12, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@behouba

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2019

@Cnly I did not notice the existence ServerName type before. So i wonder if Origin field of RespSendJoin should be of type ServerName instead of string.

Dendrite configuration struct containt ServerName field of type gomatrixserverlib.ServerName.

@Cnly

This comment has been minimized.

Copy link
Collaborator

commented Oct 13, 2019

@behouba Oops... I didn't notice that during my review. 🤦 It should be ServerName. Could you fix that in a new PR please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.