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 :)
Le lundi 29 janvier 2018 17:39:27 UTC+1, matthe...@gmail.com a écrit :
>
> 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
>
> On Monday, January 29, 2018 at 2:13:24 AM UTC-6, Asticode wrote:
>>
>> 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
>>
>
--
You received this message because you are subscribed to the Google Groups
"golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to golang-nuts+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
