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

fix(station): corrected SDk to SDK #255

Merged
merged 3 commits into from Aug 10, 2022

Conversation

bsmirks
Copy link
Contributor

@bsmirks bsmirks commented Aug 8, 2022

This is a fix for #252 in which I found instances of the word SDk and replaced with SDK. Please let me know if anything was missed in the contribution workflow. Thanks!

Copy link
Contributor

@OrMemphis OrMemphis left a comment

Choose a reason for hiding this comment

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

It's better if this message was a constant for the whole package, maybe call it invalidHeaderErrMessage. you can place it anywhere you see fit

@OrMemphis
Copy link
Contributor

Hey @bsmirks, all good with the contribution workflow!
I had a minor suggestion before merging, thanks a lot for your contribution.

@bsmirks
Copy link
Contributor Author

bsmirks commented Aug 9, 2022

Sounds good, I will play with implementing this and update the PR accordingly.

@bsmirks bsmirks requested a review from OrMemphis August 9, 2022 21:41
@bsmirks
Copy link
Contributor Author

bsmirks commented Aug 9, 2022

I've updated poison_messages.go before the first function is defined to contain a constant which stores the error message, then called it where needed.

Copy link
Contributor

@OrMemphis OrMemphis left a comment

Choose a reason for hiding this comment

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

Nice, although my naming may have confused a little, I think we can include that "Error while getting notified..." in the constant if it's not being used in parts anywhere else.
that said, probably the name should suggest that it only comes from poison messages, like invalidPosionHeaderErrMessage

@@ -31,6 +31,8 @@ import (

type PoisonMessagesHandler struct{}

const invalidHeaderErrMessage string = "Missing mandatory message headers, please upgrade the SDK version you are using"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const invalidHeaderErrMessage string = "Missing mandatory message headers, please upgrade the SDK version you are using"
const invalidPoisonHeaderErrMessage string = "Error while getting notified about a poison message: Missing mandatory message headers, please upgrade the SDK version you are using"

@@ -54,7 +56,7 @@ func (pmh PoisonMessagesHandler) HandleNewMessage(msg *nats.Msg) {
producedByHeader := poisonMessageContent.Header.Get("producedBy")

if connectionIdHeader == "" || producedByHeader == "" {
logger.Error("Error while getting notified about a poison message: Missing mandatory message headers, please upgrade the SDk version you are using")
logger.Error("Error while getting notified about a poison message: " + invalidHeaderErrMessage)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logger.Error("Error while getting notified about a poison message: " + invalidHeaderErrMessage)
logger.Error(invalidPoisonHeaderErrMessage)

Comment on lines 598 to 599
logger.Error("Error while getting notified about a poison message: " + invalidHeaderErrMessage)
c.AbortWithStatusJSON(configuration.SHOWABLE_ERROR_STATUS_CODE, gin.H{"message": "Error while getting notified about a poison message: " + invalidHeaderErrMessage})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logger.Error("Error while getting notified about a poison message: " + invalidHeaderErrMessage)
c.AbortWithStatusJSON(configuration.SHOWABLE_ERROR_STATUS_CODE, gin.H{"message": "Error while getting notified about a poison message: " + invalidHeaderErrMessage})
logger.Error(invalidPoisonHeaderErrMessage)
c.AbortWithStatusJSON(configuration.SHOWABLE_ERROR_STATUS_CODE, gin.H{"message": invalidPoisonHeaderErrMessage})

@OrMemphis
Copy link
Contributor

OrMemphis commented Aug 10, 2022

@bsmirks thanks! :)

consider my suggestions;
if these messages are being used in parts, tell me and I'll approve and merge.
else, you can commit my suggestions and I'll approve.

btw you are more than welcome to our Discord server, I'm there if you have any question

@bsmirks
Copy link
Contributor Author

bsmirks commented Aug 10, 2022

I have made a change to the name of the constant as suggested as I think that making the constant more descriptive is worthwhile in the long run for readability's sake.

However, I think that the bit about "Error while getting notified about..." should stay as is since it is being used in parts elsewhere like here which is where I got the convention for leaving it there and appending the constant afterwards. Let me know what you think!

Joined the Discord :)

@OrMemphis OrMemphis merged commit 5a73666 into superstreamlabs:staging Aug 10, 2022
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

2 participants