[ANN] Bob, a framework to create a distributed AI that can learn to understand your voice, speak back, interact with your mouse and keyboard, and anything else you want

[Asticode]

Heys guys,

I'm happy to announce Bob, a framework to create a distributed AI that can learn to understand your voice, speak back, interact with your mouse and keyboard, and anything else you want: https://github.com/asticode/go-astibob.

Let me know what you think.

Cheers

[matthe...@gmail.com]

Hi Quentin, here’s a code review.

Since type ability is not exported, why not embed sync.Mutex? What does o stand for?

I’d say that comments like “// newAbility creates a new ability” are unnecessary. The identifiers are documentation. There are many unnecessary comments.

For read-only methods like *ability.isOn() perhaps a sync.RWMutex may make sense.

type brain could have sync.Mutex and *astiws.Client embedded. type brains, type dispatcher, type interfaces could have sync.Mutex embedded. type server could have everything besides name embedded. type brainsServer could have everything embedded. type clientsServer could have everything besides stopFunc embedded. The sub-packages have more cases like this.

There are no tests.

The horizontal dependency of go-astibob/abilities to go-astibob/brain is a red flag for me. Why not define the abilities in package astibob?

To me the demo examples show a clean interface for interacting with the library.

I don’t understand the pkg directory, why not have these as regular sub-packages?

In go-astibob/pkg/portaudio, robotgo, I’m not sure what an alternative would be, but defining methods on a struct{} type seems unnecessary compared to just having functions. Elsewhere I’ve suggested defining a struct type with function fields instead. If this is to satisfy an interface, perhaps there is another approach where the interface is unnecessary.

These are the most complex html templates I’ve seen, I may refer back to these sometime.

The godoc is a reasonable size but there are a lot of types without any functionality. The README.md seems long, perhaps examples/demo could contain some of that information.

Matt

[Asticode]

First off, cheers for the code review :)

>> Since type ability is not exported, why not embed sync.Mutex?

I agree.

>> What does o stand for?

I named this variable "isOn" first, but then I named the function to access it the same way, therefore I had to find another name. I do agree "o" sucks, I'll try to find another clearer way.

>> I’d say that comments like “// newAbility creates a new ability” are unnecessary. The identifiers are documentation. There are many unnecessary comments.

I agree.

>> For read-only methods like *ability.isOn() perhaps a sync.RWMutex may make sense.

I agree.

>> type brain could have sync.Mutex and *astiws.Client embedded. type brains, type dispatcher, type interfaces could have sync.Mutex embedded. type server could have everything besides name embedded. type brainsServer could have everything embedded. type clientsServer could have everything besides stopFunc embedded. The sub-packages have more cases like this.

I agree even though I've never thought like this. For my own curiosity, what gain do you see by embedding structs like this?

>> There are no tests.

I agree :(

>> The horizontal dependency of go-astibob/abilities to go-astibob/brain is a red flag for me. Why not define the abilities in package astibob?

I can't see any reasons why not, therefore that may be a good idea.

>> I don’t understand the pkg directory, why not have these as regular sub-packages?

I wanted to keep packages using CGO in a separate place since astibob, astibrain and abilities use interfaces to interact with features. Having one root folder for those packages was more convenient for me. But you may be right.

>> In go-astibob/pkg/portaudio, robotgo, I’m not sure what an alternative would be, but defining methods on a struct{} type seems unnecessary compared to just having functions. Elsewhere I’ve suggested defining a struct type with function fields instead. If this is to satisfy an interface, perhaps there is another approach where the interface is unnecessary.

This is to satisfy interfaces indeed. If you have an alternative solution, I'd be happy to hear it.

>> The godoc is a reasonable size but there are a lot of types without any functionality

What do you mean by that?

>> The README.md seems long, perhaps examples/demo could contain some of that information.

Same, what do you mean by that?

Again, cheers for the code review :)

[matthe...@gmail.com]

I agree even though I've never thought like this. For my own curiosity, what gain do you see by embedding structs like this?

Now when making calls you don’t have to refer to the field to call the methods; the embedded type’s methods are promoted to the struct type but still use the field value in the method. In my projects I’ve liked this as it has the code use less words (t.Lock() instead of t.m.Lock()), although I can see that there may be cases where referring to the struct field explicitly is preferable.

This is to satisfy interfaces indeed. If you have an alternative solution, I'd be happy to hear it.

My thought is that the interface itself should be changed since the methods it calls have no need for data in the underlying type. Here’s an alternative representation I suggested in another code review: https://play.golang.org/p/F_f0hLNsmSW

What do you mean by that?

I may have misunderstood the use of the types as inputs to other pieces, but perhaps refactoring the types could simplify the library.

Same, what do you mean by that?

A concise README.md may help people use the library. Moving some of the documentation to sub-folders like demo and leaving a reference in the readme may be as effective without so much scrolling.

Thanks for sharing Bob here.

Matt