Rename getter functions to be idiomatic #8320
Conversation
terencechain
changed the title
There was a problem hiding this comment.
I would like to propose the following standard for naming constructors.
If the package name describes the object being created, then the constructor name should be New, to avoid duplication. Example: service.New vs service.NewSevice (the function returns a Service struct). Otherwise, the name should be NewXyz. Example: helpers.NewSlotTicker vs helpers.New. Seeing helpers.New does not tell you anything about what's being created.
beacon-chain/db/kv/blocks.go
Outdated
| // getBlockRootsByFilter retrieves the block roots by slot | ||
| func getBlockRootsBySlot(ctx context.Context, tx *bolt.Tx, slot uint64) ([][]byte, error) { | ||
| ctx, span := trace.StartSpan(ctx, "BeaconDB.getBlockRootsBySlot") | ||
| // blockRootsByFilter retrieves the block roots by slot |
There was a problem hiding this comment.
| // blockRootsByFilter retrieves the block roots by slot | |
| // blockRootsBySlot retrieves the block roots by slot |
beacon-chain/p2p/service.go
Outdated
| @@ -83,7 +83,7 @@ type Service struct { | |||
| } | |||
|
|
|||
| // NewService initializes a new p2p service compatible with shared.Service interface. No | |||
| // connections are made until the Start function is called during the service registry startup. | |||
| // connections are made until the New function is called during the service registry startup. | |||
There was a problem hiding this comment.
Are you sure this change is correct?
beacon-chain/p2p/service.go
Outdated
| @@ -83,7 +83,7 @@ type Service struct { | |||
| } | |||
|
|
|||
| // NewService initializes a new p2p service compatible with shared.Service interface. No | |||
| // connections are made until the Start function is called during the service registry startup. | |||
| // connections are made until the New function is called during the service registry startup. | |||
| func NewService(ctx context.Context, cfg *Config) (*Service, error) { | |||
There was a problem hiding this comment.
I would prefer New instead of NewService. Similarly for other services.
There was a problem hiding this comment.
Yes. But let's do it in another PR
| @@ -103,7 +103,7 @@ func (bs *Server) ListAttestations( | |||
| } | |||
|
|
|||
| // ListIndexedAttestations retrieves indexed attestations by block root. | |||
| // IndexedAttestationsForEpoch are sorted by data slot by default. Start-end epoch | |||
| // IndexedAttestationsForEpoch are sorted by data slot by default. New-end epoch | |||
There was a problem hiding this comment.
Something is not right here.
shared/slotutil/slotticker.go
Outdated
| // GetSlotTicker is the constructor for SlotTicker. | ||
| func GetSlotTicker(genesisTime time.Time, secondsPerSlot uint64) *SlotTicker { | ||
| // New starts and returns a new SlotTicker instance. | ||
| func New(genesisTime time.Time, secondsPerSlot uint64) *SlotTicker { |
There was a problem hiding this comment.
I disagree with this change - slotutil.New does not convey information about what is being created. I would prefer slotutil.NewSlotTicker.
endtoend/helpers/epochTimer.go
Outdated
| // GetEpochTicker is the constructor for EpochTicker. | ||
| func GetEpochTicker(genesisTime time.Time, secondsPerEpoch uint64) *EpochTicker { | ||
| // New starts the EpochTicker. | ||
| func New(genesisTime time.Time, secondsPerEpoch uint64) *EpochTicker { |
There was a problem hiding this comment.
I disagree with this change - helpers.New does not convey information about what is being created. I would prefer helpers.NewEpochTicker.
shared/slotutil/slotticker.go
Outdated
| // entering a offset greater than secondsPerSlot is not allowed. | ||
| func GetSlotTickerWithOffset(genesisTime time.Time, offset time.Duration, secondsPerSlot uint64) *SlotTicker { | ||
| func NewWithOffset(genesisTime time.Time, offset time.Duration, secondsPerSlot uint64) *SlotTicker { |
There was a problem hiding this comment.
We don't know what is that thing with offset. I would prefer NewSlotTickerWithOffset.
What type of PR is this?
What does this PR do? Why is it needed?
Rename a few getter functions to make it more idiomatic for Go
More info: https://hackmd.io/CVorlf6HQpWtU5jCSPQvhg
Which issues(s) does this PR fix?
N/A
Other notes for review
N/A