Remove unused parameter from function logger.setupFileLogger

[miller-productions]

Hi there,

Suggesting removal of an unused paramater in mean/config/lib/logger.js

Down in the function logger.setupFileLogger = function setupFileLogger(options) {

Notice the function parameter options?

• It isn't used anywhere within that function.
• At the one place where logger.setupFileLogger is called, no argument is passed.

Very minor issue, essentially just a tidy-up.

What does everyone think?

[hyperreality]

I think it's customary to leave such parameters in, just to make clear to future readers of the code that options can be passed to the function.

[mleanos]

There was some work done with this functionality recently, to clean up the implementation.

c8cbcd3

The support for passing options was left in place, so perhaps this was just missed. We could pass in the options here:

mean/config/lib/logger.js

Line 44 in c8cbcd3

var fileLoggerTransport = this.getLogOptions();

@lirantal Was it intentional to not pass the options here, or was it just missed?

[lirantal]

Looked at it but I can't remember why I left it there. If it's unused then we can move it out.

[simison]

btw shouldn't eslint catch un-used parameters?

[lirantal]

No, because it's disabled.
Do you think it's worth to warn or error on detecting this?

[simison]

Do you think it's worth to warn or error on detecting this?

I'd say error. Others?

[mleanos]

We should add the options to this call:

mean/config/lib/logger.js

Line 44 in c8cbcd3

var fileLoggerTransport = this.getLogOptions();

We need to provide the option for our users to pass in their own configuration for Winston to logger.setupFileLogger. For this to work as expected, we'd need to update this portion to "extend" (merge) the settings passed in as the options param & then env config.

mean/config/lib/logger.js

Line 77 in c8cbcd3

var _config = _.clone(config, true);

Also, why are we bothering to pull in the whole config here? We should just use the section of the config that we're concerned with (config.log).

IMO, eslint should throw an error in the case of an unused parameter.

[lirantal]

Good points @mleanos, I'll take a look.

[lirantal]

Setting no-unused-vars will hiccup quite a bit on issues so that's probably not the time to enable it:

[13:09:38] 
gulpfile.js
  10:3  error  'testConfig' is defined but never used  no-unused-vars

server.js
  7:5  error  'server' is defined but never used  no-unused-vars

config/config.js
  125:49  error  'assets' is defined but never used  no-unused-vars

config/env/production.js
  3:5  error  'fs' is defined but never used  no-unused-vars

config/lib/express.js
  172:54  error  'app' is defined but never used  no-unused-vars

config/lib/logger.js
  42:51  error  'options' is defined but never used  no-unused-vars

config/lib/mongoose.js
  23:7  error  '_this' is defined but never used  no-unused-vars

config/lib/seed.js
   7:3   error  'crypto' is defined but never used  no-unused-vars
  58:43  error  'reject' is defined but never used  no-unused-vars
  72:11  error  'User' is defined but never used    no-unused-vars

config/lib/socket.io.js
  81:70  error  'err' is defined but never used  no-unused-vars

modules/users/server/controllers/users/users.authentication.server.controller.js
  206:51  error  'next' is defined but never used  no-unused-vars

modules/core/client/config/core.client.route-filter.js
  14:68  error  'fromParams' is defined but never used  no-unused-vars

modules/core/client/config/core.client.routes.js
  23:55  error  '$location' is defined but never used  no-unused-vars

modules/core/client/controllers/home.client.controller.js
  9:9  error  'vm' is defined but never used  no-unused-vars

modules/core/client/directives/show-errors.client.directives.js
  36:11  error  'initCheck' is defined but never used  no-unused-vars

modules/users/client/directives/password-verify.client.directive.js
  20:11  error  'status' is defined but never used  no-unused-vars

modules/articles/client/controllers/admin/article.client.controller.js
  39:32  error  'res' is defined but never used  no-unused-vars

modules/users/client/controllers/settings/change-password.client.controller.js
  27:79  error  'response' is defined but never used  no-unused-vars

modules/articles/tests/server/admin.article.server.routes.tests.js
    3:5   error  'should' is defined but never used          no-unused-vars
   66:33  error  'signinRes' is defined but never used       no-unused-vars
   79:42  error  'articleSaveRes' is defined but never used  no-unused-vars
  111:33  error  'signinRes' is defined but never used       no-unused-vars
  118:13  error  'userId' is defined but never used          no-unused-vars
  161:33  error  'signinRes' is defined but never used       no-unused-vars
  168:13  error  'userId' is defined but never used          no-unused-vars
  188:33  error  'signinRes' is defined but never used       no-unused-vars
  195:13  error  'userId' is defined but never used          no-unused-vars
  230:9   error  'articleObj' is defined but never used      no-unused-vars
  235:33  error  'signinRes' is defined but never used       no-unused-vars
  242:13  error  'userId' is defined but never used          no-unused-vars

[mleanos]

Ok. Definitely not the time to enable. We'll have to look at these later. Probably after 0.5.0 is released.

[mleanos]

@lirantal We won't add the eslint option right now, but I think we should add options to the call here:

mean/config/lib/logger.js

Line 44 in c8cbcd3

var fileLoggerTransport = this.getLogOptions();

I consider this a bug, since our "interface" of the logger service allows for the passing of the options but is never applied.

[lirantal]

@mleanos I know what you're talking about but to be honest fixing that options argument to be passed wouldn't matter that much because the logger anyway gets called as logger.setupFileLogger({}); internally in the logger library.

[lirantal]

I'll clean that part up but because anyway the log options are taken from the config object it doesn't seem to make sense to pass more options to it.

[mleanos]

@lirantal I was more referring to how the getLogOptions(configOptions) method accepts a config parameter, and since we're passing options into setupFileLogger(options), then we should "merge" the environment config & the provided configOptions here:

mean/config/lib/logger.js

Line 79 in c8cbcd3

_config = configOptions;

This way if any of our users want to override, or add an additional, option to the Winston logger, all they would need to do is pass it in at this line:

mean/config/lib/logger.js

Line 144 in c8cbcd3

logger.setupFileLogger({});

For instance:

logger.setupFileLogger({ myCustomOption: value });

This way they wouldn't have to modify any of the code inside the Logger service. It's not a huge deal to me if we add this capability or not. However, I just wanted to make sure my point was clear.

[lirantal]

@mleanos I get what you mean but externally calling logger.setupFileLogger({myOptions}) isn't going to setup a different and custom file logger because that function is not a singleton, it will just try to add another custom logger, in addition to whatever was instantiated already (which might seem like a feature but I don't think this is what we intended).

See what I mean? Do you think it's still worth to chase this change or just clean out the extra options for now?

[mleanos]

I didn't mean that it would be called externally. Someone could pass options directly on this exact line in the logger file, instead of passing an empty object:

mean/config/lib/logger.js

Line 144 in c8cbcd3

logger.setupFileLogger({});

I'm ok with just having you clean it up. As it's not a critical issue to me.

[lirantal]

@mleanos I'm not trying to run around the tree, I value your feedback, but my thinking is that this kind of change that someone will make will cause them conflicts in future syncs with the project so it's probably one of the framework setups that people usually tend not to change.

Let's clean it up first and can further refactor after 0.5.0, you're most welcome to also propose the change :)

[mleanos]

..this kind of change that someone will make will cause them conflicts in future syncs with the project so it's probably one of the framework setups that people usually tend not to change.

I agree.

This isn't a big deal to me, just thought it would be a convenience. However, any persistent settings can just be added to the default env config. So in the end, I don't see this very useful either. I guess I just figured that since the options parameter was there, it should do something. Let's just clean that up.

[lirantal]

Cool, thanks for futher confirming 👍