Hexagon / node-telldus

Node bindings for telldus-core
Other
34 stars 10 forks source link

Node method naming convention #13

Closed nilzen closed 10 years ago

nilzen commented 10 years ago

I think the methods exposed to node should be named according to the naming convention Node uses.

Ex.

turnOn -> turnOnSync turnOff -> turnOffSync dim -> turnOffSync

And the other way:

asyncTurnOn -> turnOn asyncTurnOff -> turnOff asyncDim -> dim

I know there are a lot more sync than async methods atm but this gives the developer a good "warning" that the method is synchronous.

Thoughts?

kmpm commented 10 years ago

I have a hard time deciding... Should it be close to the library we are creating a wrapper for, so that documentation and samples from other uses then through node is more similar. Or as you suggested, go all node.

I'm leaning towards supporting your suggested "async by default" way. Since this fork is sort of a clean break from the old one, this is the time to do the change.

Hexagon commented 10 years ago

I agree, in a perfect world all functions should be async with callbacks, so i think async by default is the way to go.

Hexagon commented 10 years ago

I'm working on it :)

nilzen commented 10 years ago

Glad we can agree on this, since I believe it's the right way of doing it in Node. No dev will probably look in telldus-core.h and complain that the Node functions with the same name are async... Were you working on any more async functions @Hexagon ?

Hexagon commented 10 years ago

Yep, i'm adding some right now, the naming convention changes are already done

nilzen commented 10 years ago

After thats complete, will you publish 0.0.2 to npm? I want to work against the async ones w/o having to rename all calls in a day or two... :)

nilzen commented 10 years ago

While we are discussing code cosmetics, some functions are camelCase and some are PascalCase. I'm no C++ dev so I have no opinion other than that there should be one way of writing it.

As I said I'm no C++ dev but I like "pretty" code, but should Batton not be Baton?

Hexagon commented 10 years ago

Most of this solved in @09a2cb7b93c0cddcc6e0241db12c5d2b2de827a3

Some notes:

Hexagon commented 10 years ago

New version released (0.0.2 on npmjs), some work left to do (as stated above), but closing this as the main issue is resolved.

nilzen commented 10 years ago

Well done!

Hexagon commented 10 years ago

@marchaos , I would reaaally like if you could help me with getting getDevicesSync converted to async. I really don't get how these handles and stuff is supposed to work. I've tried everything and get segfaults every time :-1:

marchaos commented 10 years ago

Hey Hexagon, Sorry I missed your email. I believe that I had the same issues (segfaults). I'm not primarily a C++ developer. I did attempt get to libuv working at some point, but kept getting segfaults.