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 CQRS marshalers #78

Merged
merged 6 commits into from
May 25, 2019
Merged

Add CQRS marshalers #78

merged 6 commits into from
May 25, 2019

Conversation

sagikazarmark
Copy link
Contributor

No description provided.

return m.marshaler().Name(v)
}

func (m *NameMarshaler) NameFromMessage(msg *message.Message) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, I'm not sure if the marshaller should have this method. Maybe I'm losing some context here but for me, it should go to message.Message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's part of the CommandEventMarshaler interface

}

func (m *StructNameMarshaler) Name(v interface{}) string {
segments := strings.Split(fmt.Sprintf("%T", v), ".")
Copy link
Member

Choose a reason for hiding this comment

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

if v is pointer it will have * prefix, do we want to remove it? 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, but it gets removed: https://play.golang.org/p/97dR-e246vB

Copy link
Member

@roblaszczak roblaszczak May 24, 2019

Choose a reason for hiding this comment

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

ah, of course, it is :)

// It wraps another marshaler which takes care of the message serialization/deserialization.
// If none provided, it falls back to JSON marshaler.
type StructNameMarshaler struct {
Marshaler CommandEventMarshaler
Copy link
Member

Choose a reason for hiding this comment

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

after some thinking, maybe it will be a better idea to not add it as an implementation of Marshaler interface, but like function and add an extra attribute for existing marshallers, like:

type JSONMarshaler struct {
	NewUUID func() string
        GenerateName func(v interface{}) string
}

with default GenerateName = ObjectName.
In that case, you may rename object.go to something more meaningful, and keep all names generations methods like functions

In that case, the implementations are still more about output format, and naming is decoupled.

What do you think? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me. I will update the PR soon


// StructName name returns struct name in format [type name].
// It ignores if the value is a pointer or not.
func StructName(v interface{}) string {
Copy link
Member

Choose a reason for hiding this comment

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

maybe ObjectNameWithoutPackage? it is pretty long, but explicit :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was rather thinking about renaming ObjectName to FullyQualifiedStructName and leave this as is, but ObjectNameWithoutPackage works as well.

Copy link
Member

Choose a reason for hiding this comment

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

ok, like I like the idea of FullyQualifiedStructName :)

// Name() string
// }
// It ignores if the value is a pointer or not.
func MessageName(fallback func(v interface{}) string) func(v interface{}) string {
Copy link
Member

Choose a reason for hiding this comment

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

what do you think about NamedObject?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following the pattern I suggested above, this could be NamedStruct, but if we stick with object, NamedObject works.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@roblaszczak roblaszczak merged commit cc6954d into ThreeDotsLabs:master May 25, 2019
@sagikazarmark
Copy link
Contributor Author

🎉 Thanks @roblaszczak

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

Successfully merging this pull request may close these issues.

None yet

3 participants