@php-coder in your commit comment you mentioned "don"t inherit ~30 methods while we"re using just two". There"s no inheritance in Go, just embedding. See https://golang.org/ref/spec#Struct_types.

When we embed a type T in a struct type "S", by not giving it an explicit name, we cause (some of) its fields and methods to be promoted.

It might be just your IDE code-completion giving the feeling that "inherit" ~30 methods vs. "less". The struct type will have the same size either way, again, there"s no inheritance.

There"s one difference though. Before this patch, the exported type DefaultCleaner had fields FileSystem and Docker, plus all exported fields of those two latter types. After the change, we"re potentially breaking users by changing the public exported API of the package, since now DefaultCleaner has no exported fields whatsoever.

Were you aware of that? That"s the whole point of me bothering to comment 😄

I suspect all we might care about is DefaultCleaner.Cleanup, so maybe there"s no need to revert this.
I really didn"t enjoy when I came across similar breaking changes in beego that broke our golang-ex...

Thank you, I knew that there is no inheritance, but it still looks similar -- struct has additional methods from another struct.

Before this patch, the exported type DefaultCleaner had fields FileSystem and Docker, plus all exported fields of those two latter types

I didn"t know about these 2 fields. Do you mean that I could write like this?

c.FileSystem.RemoveDirectory(config.WorkingDir)

After the change, we"re potentially breaking users by changing the public exported API of the package, since now DefaultCleaner has no exported fields whatsoever.

Yes, that was my intention because we"re using just 2 and "promoting" 30.

Thank you, I knew that there is no inheritance, but it still looks similar -- struct has additional methods from another struct.

Before this patch, the exported type DefaultCleaner had fields FileSystem and Docker, plus all exported fields of those two latter types
I didn"t know about these 2 fields. Do you mean that I could write like this?

c.FileSystem.RemoveDirectory(config.WorkingDir)

Yes! Normally the point of embedding is that you don"t need the extra dot.

These are equivalent:

// using promoted method
c.RemoveDirectory(config.WorkingDir)

// using explicit method
c.FileSystem.RemoveDirectory(config.WorkingDir)

I think the original usage of embedding here was not very idiomatic nor necessary.

One common usage of that is to add a mutex to control access to some struct fields, example:

// Counter is just a silly example, never write a counter like this.
type Counter struct {
    sync.Mutex
    count int64
}

func (c *Counter) Up() {
    // Call promoted method from sync.Mutex.
    // Equivalent to c.Mutex.Lock()
    c.Lock()
    defer c.Unlock()
    c.count++
}

After the change, we"re potentially breaking users by changing the public exported API of the package, since now DefaultCleaner has no exported fields whatsoever.

Yes, that was my intention because we"re using just 2 and "promoting" 30.

That"s why IMHO it"s better to default to unexported, and only export things that have reason to be exported. That gives us freedom to refactor without breaking APIs.

I would actually argue (but reasonable minds are welcome to differ) that the primary reason to embed one struct in another is to effectively make the parent struct be able to serve as (be passed as) the child struct. It"s composition"s equivalent to inheritance. So in the case where you embed the Mutex above, i don"t think that"s actually a good reason to do it, unless you have a need to pass Counter to functions that take Mutexes, just like in an inherited language you probably wouldn"t make Counter extend or implement Mutex (though you might, certainly). If you just need a Mutex on Counter, then making the mutex a field and calling "c.mutex.Lock()" seems reasonable to me, though I won"t argue w/ the convenience embedding gives you to just call c.Lock().

At the end of the day though, I would agree that the default preference should be to not embed things.

actually i can even think of a reason you would not want to embed the mutex in this way: you"ve now exposed the mutex to any code w/ a reference to your Counter, which means it could potentially lock or unlock the mutex in unsafe ways. violates data encapsulation.

effectively make the parent struct be able to serve as (be passed as) the child struct

@bparees this is not true, because the types are different!
What may come close to that is transitively implementing interfaces.

If T is embed in S and T implements I, then S implements I too.

Sorry yes, I should have said interface and not struct, but the point I was trying to make is that if you have a function that takes Interface I, and you want to pass S as an instance of I, and T implements I, then that is a good reason to embed T (over including it as a norma field in S). Otherwise all you"re doing is saving some typing in exchange for the dubious benefit of exposing all the behavior of T at the top level of S, which as I noted in your Counter example, can be undesirable.