This repository has been archived by the owner on Jun 23, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
asdine
requested review from
yaziine and
tealeg
and removed request for
yaziine
December 27, 2018 10:46
yaziine
reviewed
Dec 28, 2018
asdine
force-pushed
the
message-formatter
branch
2 times, most recently
from
January 7, 2019 16:09
2749675
to
4c92e63
Compare
Co-Authored-By: asdine <asdine.elhrychy@gmail.com>
asdine
force-pushed
the
message-formatter
branch
from
January 8, 2019 09:10
2794bfa
to
c5e2194
Compare
tealeg
suggested changes
Jan 9, 2019
codec/codec_test.go
Outdated
"github.com/stretchr/testify/require" | ||
) | ||
|
||
// checks if codec encodes to the proper output. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🌷 The "checks if" at the start of all these test descriptions is redundant. We know they're checking something. It's better to just make a plain statement. Also, this sort of case might not even need a comment at all (I often write them also, but I don't think "it does the right thing" is adding anything much).
Co-Authored-By: asdine <asdine.elhrychy@gmail.com>
🆙 |
🆙 @tealeg |
tealeg
approved these changes
Jan 9, 2019
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good - I've made suggestions for a couple of minor fixes in the English in the doc.go
, but otherwise excellent, and you can merge when they're applied.
Co-Authored-By: asdine <asdine.elhrychy@gmail.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces the message formatters and unformatters to Felice.
Producer
MessageConverter
interfaceMessageConverterV1
is the default implementation of the MessageConverterConsumer
MessageConverter
interfaceMessageConverterV1
is the default implementation of the MessageConverterOther changes
message
package has been removed. It was difficult to have one type for both the producer and consumer while keeping this type coherent. I created oneMessage
for both packages which is specialized for use in its package.handlers
had been removed and its code was integrated in theconsumer
package. With the removal of themessage
package, we had an import cycle between theconsumer
package and thehandlers
package, which forced me to reconsider the organization of this package.I think for now a Handler is strongly coupled with a Consumer and it is normal to have them in the same package. I also unexported the
Collection
type as I don't yet see how it can be useful to be used outside of this package. Let me know if you disagree