-
Notifications
You must be signed in to change notification settings - Fork 0
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
Review fixes of #872 #7
Conversation
1. Improved the name of certain variable 2. Improved spacing
1.Changes the names of few variables 2.Improved code complexity
1.Changed the name of few variables
1.Changed the way to store assignee, mention, reporter and watching in struct so that the code can be easily tested
1. Changed the name of various variables 2. Used constants everywhere
1. Added Test cases 2. Used booleans instead of pointer in RolesForDMNotification
1. Modified test cases
1. Self reviewed
1. Changed the formatting 2. Changed the name of few variables 3. Improved code quality
1. Improved code quality
1. Improved code quality
1. Improved code quality
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.
Thank you! just a small detail on a commented change and a couple of questions to get a bit more of context, let me know! 👍
// be used to set the default instance. | ||
// - The instances themselves should be forward-compatible, including | ||
// CurrentInstance. | ||
// - v2keyKnownJiraInstances ("known_jira_instances") was stored as a |
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.
I think we can restore the changes in this file, there aren't any.
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.
@javaguirre , I was getting an error : File is not gofmt -ed with -s
from server/kv.go
on running make check-style
command because the file was not properly formatted according to Go rules. So I run gofmt -s -w ./server
for formatting.
} | ||
|
||
// Check old setting for backwards compatibility | ||
return s.Notifications | ||
} | ||
|
||
func (p *Plugin) fetchConnectedUserFromAccount(account map[string]string, instance Instance) (Client, *Connection, error) { |
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.
Why is this needed?
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.
@javaguirre because this was one of your review comments
api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return(nil) | ||
api.On("LogError", mockAnythingOfTypeBatch("string", 10)...).Return(nil) | ||
api.On("LogError", mockAnythingOfTypeBatch("string", 13)...).Return(nil) | ||
api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return() |
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.
why it was nil before and now it's not? It doesn't seem like a needed change, but I might be wrong.
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.
@javaguirre because the function LogError
and LogDebug
does not have a return type.
Sorry, I didn't see I was in your fork, I will wait until you send the PR to the MM repo 👍 |
Review fixes of #872