Removed log4js dependency.

[xytis]

Changes:

• log4js is no longer added as npm dependency
• default logger is 'console' object

• logger injection can be done during initialization, or later as:

  nami.logger = mostAmazingLogger;

[marcelog]

Hello :)

Thanks for the contribution! I like the change, it makes sense :)

Question: How about instead of adding a 2nd argument to the constructor, we allow the logger to be set right inside the first argument, so instead of it being 'namiConfig' we can just call it 'config', and add a property 'logger' to it? So we can set the logger right away when creating the nami client (achieving 2 things: on one hand, consistency, so the nami client is fully configured -logger included- when instantiated, and on the other hand, we don't change the api that much, and allow other options to be set later on). Also, there's an example inside src/index.js that should be updated for this change.

Thoughts?

[xytis]

Sure, why not. I just thought that renaming 'namiConfig' is a breaking change.

I will redo the PR soon. Also I did change index.js.

[marcelog]

right, i missed the change, sorry. why do you think that changing namiConfig would be a breaking change exactly? it's only intended to be used as an argument for the constructor and then only used internally inside the client. but i'm interested in your comments :)

[xytis]

I was not sure what were You planning to do with namiConfig. For example adding a bunch of asterisk related parameters (of which I have no idea).

Either way, sticking 'logger' in the first hash would be a nice alternative.

[xytis]

Oh, and there is another issue: If I add logger in the first hash, I either would like to copy other parameters away (like making a deep copy of understood params), or calling a 'delete config.logger' on input hash, to remove duplicate reference to the same logger instance.

[xytis]

And a final issue: If we have a 'config' object, it should be loadable from file. Where logger instance is not loadable from file. So after some thought -- I vote on sticking to two diferent objects, one for actual config, which stays loadable as .json for example:

 nami = new Nami(require('../nami.config.json'));

And another optional object which adds instant access to Nami internals (note that all internals can be altered after initialization, if required.)

[marcelog]

It's actually very rare to have your namiconfig in a specific file. Usually your app will have a config file on its own, with your own format, perhaps with a nami section that one will extract or reference explicitely and feed into the nami client (you wouldn't like to actually pass al your app options to nami, just the section needed). So it's not that much trouble to attach the logger instance into those options once parsed, imho

[xytis]

I do live in a magical world, where rare things happen all the time =] http://xkcd.com/1172/ And I do have an application which loads whole config from a file, then passes a piece of it to Nami. This goes like this:

 var config = require('config.json');
 var nami = new Nami(config.nami);

I do agree that there is no big diference between:

 config.nami.logger = console;
 var nami = new Nami(config.nami);

and

 var nami = new Nami(config.nami, { logger: console });

So just say which way You prefer and I'll fix the PR =].

[marcelog]

I think I like the first one, where we attach the logger to the nami options once the config object has been setup. To sum up my point of view: It doesn't make much sense to me to have 2 separate objects to specify options (asterisk options and nami options). I would actually support a 2nd argument in the constructor to inject the logger on its own, but not a whole set of options. So let's go with the first one, unless there's something else odd enough to discuss :) I'll be happy to merge once I give it a try locally :)

[marcelog]

@xytis i'm curious... have you tried this patch? the console object does not have methods like debug, so i don't thinks this patch will work as it is

[xytis]

I tested only with another logger, did not test default option. Commited another fix for my shortsightedness.

[xytis]

Ups =]

[carragom]

This would be useful. Is there anything preventing this merge ?

[ellisgl]

This would be super useful

[kubrey]

Why still not merging this? log4js 0.3.9 is using sys instead of util, which is painful due to constant notices of deprecation

[marcelog]

Hi guys,

Sorry for the delay, I had other projects to take care of. I've merged these changes and also added the logLevel property, so one can also control the log level of the default logger embedded with Nami ([49b7b3489968a051a5934bed682d54c5c7b5a179]). Could you check it out and let me know your comments?

@kubrey nice avatar, btw! I hope that the log4js warnings didn't affect your bodily fluids too much.

Cheers,