search

hybridgroup / cylon-cli

Cylon.js Command Line Interface
http://cylonjs.com
Other
8 stars 1 forks source link

Fix issues with cylon generate module. #13

Closed edgarsilva closed 10 years ago

edgarsilva commented 10 years ago

This fixes #11

deadprogram commented 10 years ago

@stewart does this match the current dev commands?

stewart commented 10 years ago

No, and stuff like https://github.com/hybridgroup/cylon-cli/blob/fix-generator/lib/commands/support/generate/module/lib/adaptor.js.tpl#L25 would just error out (where is adaptor defined?).

I don't know how to deal with commands in new modules, since we don't know how users will deal with them (lib/commands.js or inline). By default I'd just have them in the Driver constructor as this.commands = {}.

deadprogram commented 10 years ago

Sounds good. @edgarsilva can you change that please?

edgarsilva commented 10 years ago

O yeah, my bad it should be Adaptor, same with drive, should be Driver.

@stewart We already discussed the commands last week, it was your suggestion to add it to the prototype.

in short what should we do about it.

stewart commented 10 years ago

That was before starting the big commands refactor. For now, let's use something like this.

For bigger modules, with many commands (like ARDrone and Sphero), there's also the setupCommands helper that is also used in the constructor of subclass Drivers.

edgarsilva commented 10 years ago

Ok, are we doing this and using this syntax only for the driver or also the adaptor?

stewart commented 10 years ago

The adaptors don't need the same commands structure, since commands are never (from an API perspective at least) directly run against them. Instead they'd just have an array to be used with proxyMethods. I don't know if this should be in the generator at all and will defer to your + @deadprogram's judgement.

deadprogram commented 10 years ago

I just wanted to make sure that the code as generated is compatible with the current main code.

edgarsilva commented 10 years ago

I think at least the structure should be consistent, for the drivers including what you mentioned above looks good to me, all other adaptors use the array defined in the prototype, so I'll leave it as it is right now.

Agreed with @deadprogram which is the changes I just did suggested by @stewart, pushing now.

deadprogram commented 10 years ago

That is not correct. Is it now a hash, correct @stewart?

edgarsilva commented 10 years ago

@deadprogram yeap, I had those changes waiting inline, this should match the new syntax.

stewart commented 10 years ago

:flipper: