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

Move accounts to separate package #360

Merged
merged 5 commits into from
Sep 26, 2017
Merged

Move accounts to separate package #360

merged 5 commits into from
Sep 26, 2017

Conversation

divan
Copy link
Contributor

@divan divan commented Sep 25, 2017

This PR moves status accounts related code into separate package.

Copy link
Contributor

@themue themue left a 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this renaming?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@divan
Copy link
Contributor Author

divan commented Sep 26, 2017

Also would like proper punctuation in all comments. Currently some have, others not.

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!

@themue
Copy link
Contributor

themue commented Sep 26, 2017

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.

Copy link
Contributor

@adambabik adambabik left a 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

@themue
Copy link
Contributor

themue commented Sep 26, 2017

Good comment, struggled at acc too. Abbreviation would be acct, see http://writingexplained.org/english-abbreviations/account. Another way could be a prefix showing where the account comes from: addressedAccount (ok, it's quite long).

@divan
Copy link
Contributor Author

divan commented Sep 26, 2017

@themue @adambabik thanks, changed to acct.

@divan divan merged commit 93492cf into develop Sep 26, 2017
@adambabik adambabik deleted the refactor/accounts_package branch December 27, 2019 10:02
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.

3 participants