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

MYST-20 Communication dialog request/Response DTO models #40

Merged
merged 66 commits into from Dec 14, 2017

Conversation

Projects
None yet
4 participants
@Waldz
Copy link
Member

commented Oct 23, 2017

No description provided.

@Waldz Waldz force-pushed the feature/MYST-20-communication-dto branch from 594cf09 to 712661f Nov 14, 2017

@Waldz Waldz force-pushed the feature/MYST-20-communication-dto branch from 3daf77d to eea2a85 Dec 11, 2017

@Waldz Waldz force-pushed the feature/MYST-20-communication-dto branch from 4dce52f to daa823a Dec 11, 2017

@Waldz Waldz requested review from shroomist, tadovas and interro Dec 11, 2017

@Waldz

This comment has been minimized.

Copy link
Member Author

commented Dec 11, 2017

Improvements done, review appreciated

@tadovas

This comment has been minimized.

Copy link
Member

commented Dec 11, 2017

Is it possible to extract hardcoded nats server address/port to options somehow? Or at least to declare inner constant in nats package. Now "127.0.0.1" is lying around everywhere.

@shroomist
Copy link
Contributor

left a comment

diff too large to read, lets keep stories smaller.
don't like messagePtr interface{} -- undescribed message and response types.

func (codec *codecBytes) Unpack(data []byte, payloadPtr interface{}) error {
switch payload := payloadPtr.(type) {
case *[]byte:
*payload = data

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 11, 2017

Contributor

payload used in switch, modified in case confusing here.

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 14, 2017

Author Member

It's casting.
But we dont have byte streams yet, so I dont want to polish them without real case.

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 14, 2017

Author Member

What was the idea of story:

  • you can write custom providers/consumers
  • providers/consumers will provide DTOs of requests/responses
  • DTOs can be any structs, what are able to serialise to JSON
)

func TestCodecBytesInterface(t *testing.T) {
var _ Codec = NewCodecBytes()

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 13, 2017

Member

This test does nothing?

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 14, 2017

Author Member

This test check if struct implements wanted interface

}

func TestCodecJSONInterface(t *testing.T) {
var _ Codec = NewCodecJSON()

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 13, 2017

Member

Same as above, does it do anything?

}

func NewAddressForIdentity(identity dto_discovery.Identity) *NatsAddress {
return NewAddress("127.0.0.1", 4222, string(identity))

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 13, 2017

Member

Is it ok to hardcode "127.0.0.1" + port here?

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 13, 2017

Member

Maybe worth changing into a variable we could modify during compilation?

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 14, 2017

Author Member

It's the same how it was before, we expect queue server to run next to node&client.
We have a ticket to make it injectable

@Waldz Waldz merged commit a7e2444 into master Dec 14, 2017

@Waldz Waldz deleted the feature/MYST-20-communication-dto branch Dec 14, 2017

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