-
Notifications
You must be signed in to change notification settings - Fork 246
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
Move accounts to separate package #360
Conversation
f3386d7
to
7457c65
Compare
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.
Mostly looks reasonable. Only two comments. Also would like proper punctuation in all comments. Currently some have, others not.
// AccountManager represents account manager interface | ||
type AccountManager struct { | ||
// Manager represents account manager interface | ||
type Manager struct { |
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 this renaming?
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.
Is the Manager
implementing a public interface (and only that)? In this case the implementation should be private and the constructor function (NewManager()
) simply should return the interface.
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.
It's renamed because now it's in separate package with name accounts
. So it's a common practice to avoid names with reduntant words like rpc.RPCMessage
or account.AccountManager
.
So after moving this code into separate package, we have two options to use this type:
Without renaming: account.AccountManager
With: account.Manager
I think go lint
can spot such naming patterns and complain as well.
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.
Ah, yes. So it's indeed better.
Oh, right. I sometimes use editor to write commit messages, and sometimes just plain command line, and adding dot is almost automatic when using editor. Thanks for bringing attention to it! |
Regarding punctuation I mostly looked at comments inside the code. So e.g. some functions/methods have a trailing dot, others not. I simply orient at the way Google does it. Here all comments are simply proper English. |
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.
Changing account
to acc
might introduce some confusion what acc
stands for. I don't have a better idea, though :P
Good comment, struggled at |
@themue @adambabik thanks, changed to |
This PR moves status accounts related code into separate package.