core/scheduler: trigger duty at slot offset #516
Conversation
core/scheduler/scheduler_test.go
Outdated
| deadlines map[core.Duty]time.Time | ||
| } | ||
|
|
||
| func (d *delayer) Get() map[core.Duty]time.Time { |
There was a problem hiding this comment.
Is this exported method used or aimed to used outside the package?
There was a problem hiding this comment.
As discussed before, the type delayer isn't exported, so nothing is leaked. But the function Get is used by other things in this package. So then the method is capitalised. I prefer using private methods/fields for internal use within in a type only.
There was a problem hiding this comment.
Also since this is a test, it is impossible to use it from outside the package.
There was a problem hiding this comment.
- test of private methods can be done in _internal_test.go files, no problem for that.
- If the method is public some developer will think that it is for public consumtion and have a method that returns the struct at some point.
hence, making it public does not give us an adventage, making it private does not requires more complex code but it keeps encapsulation.
There was a problem hiding this comment.
When you make a method public you are documenting that the method is to be comsumed by other packages
There was a problem hiding this comment.
You general approach of making everything private, both types and fields/methods is a subjective opinion which I have already mentioned that I do not agree with. I feel making types private is sufficient to ensure encapsulation. Public fields/method then indicate that they are used by other things in the same package as opposed to only for internal use in the type itself.
public methods in private structs does not provide encapsulation in Go as shown in the code https://github.com/leolara/privateExperiment
There was a problem hiding this comment.
We are in a loop.
These are facts, not subjective at all:
- private methods can be tested in Go, _internal_test.go allow that
- public methods of private structs are accessible outside the package in Go, because in Go it is possible to have a public function that returns a private struct, and that makes the public methods of the struct accessible. However, private methods are never accessible.
This is a common rule in Software Engineering:
- lower visibility by default, higher visibility needs a justification. Not the other way around
There was a problem hiding this comment.
here there is an example of code: https://willhaley.com/blog/private-and-public-visibility-with-go-packages/
a private struct with public method and private method, they both have a place in Go code
There was a problem hiding this comment.
I've documented my approach here: https://github.com/corverroos/go-visibilty
There was a problem hiding this comment.
ok made it unexported.
|
|
||
| // Delay implements scheduler.delayFunc and records the deadline and returns it immediately. | ||
| func (d *delayer) Delay(duty core.Duty, deadline time.Time) <-chan time.Time { | ||
| d.mu.Lock() |
There was a problem hiding this comment.
Is this method used or aimed to be used outside the package?
There was a problem hiding this comment.
see comment above
There was a problem hiding this comment.
ok made it unexported.
Codecov Report
@@ Coverage Diff @@
## main #516 +/- ##
==========================================
- Coverage 57.42% 55.42% -2.00%
==========================================
Files 87 91 +4
Lines 8032 8383 +351
==========================================
+ Hits 4612 4646 +34
- Misses 2819 3128 +309
- Partials 601 609 +8
Continue to review full report at Codecov.
|
|
Just because However, if this was outside the |
corverroos
added
the
merge when ready
Trigger duties at an offset within each slot.
category: feature
ticket: #515