-
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
feat: default categorisation change #180
Conversation
3718d71
to
43303e7
Compare
43303e7
to
a53d065
Compare
890df53
to
e658809
Compare
// RetrieveDefaultCategorisation takes dimension, returns categorisations, and checks if any are default. | ||
// if so, it returns the relevant information. If there are no default categorisations, it returns empty string | ||
// for default categorisation, and the original dimension name and label to persist instead. | ||
func (api *API) RetrieveDefaultCategorisation(dimension *model.Dimension, datasetName string) (string, string, string, 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.
Preference for return will be either to introduce a struct or named return variables. Function signature is not obvious enough to explain what these 3 returned strings are
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 agree
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 the named variables add too many extra lines of code, and a struct feels overkill for just one function, @j-s-rawat can I just add a comment that explains the return signature?
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.
To be honest this seems like a bit of a code smell that the function is trying to do too much. It may be better to split it up into two (or more).
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.
given the need for the changes soon, this will be dealt with in a future refactor
e658809
to
108191d
Compare
a6629df
to
7ed524f
Compare
What
Describe what you have changed and why.
How to review
Describe the steps required to test the changes.
Who can review
Describe who worked on the changes, so that other people can review.