Add logger

[mattijsbliek]

This PR adds the Signale via a simple interface, so it can easily be swapped out for something else.

The log level is configurable via usersettings.json. To make these settings readily available throughout the application, I wrote a small config utility class which loads the settings on startup.

[mattijsbliek]

Noticed just now that the tests won't run due to the import.meta.url in the config class. Will see if I can find a fix somewhere this week.

[ryanolf]

Great. I’ll look at setting up some CI stuff on the repo to run tests automatically on pull requests to make catching this sort of thing easier.

On Sun, Aug 8, 2021 at 11:46 AM Mattijs Bliek @.***> wrote:

Noticed just now that the tests won't run due to the import.meta.url in the config class. Will see if I can find a fix somewhere this week.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ryanolf/node-sonos-nfc/pull/6#issuecomment-894838746, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJSBSWQR234NUOQDXREU3TT33GIFANCNFSM5BVEVXRQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

[mattijsbliek]

Nice! Tests are fixed 👍

[ryanolf]

I'm getting failed tests for process_sonos_command on this. The error is from line 100 of process_sonos_command.js and is:

TypeError: _logger.default.success is not a function

I just setup CI to run the test suite on the main branch and pull requests against it. I'm going to close and open this pull request to see if I can get it to run.

[ryanolf]

It looks like that logger.success is supposed to be a logger.info.

[ryanolf]

@mattijsbliek Tests pass by changing that logger.success to logger.info but the code doesn't run for me. The first issue seems to be that logger.js is looking for a module config but it should be config.js. With that fixed, the "Unsupported log_level" exception is thrown -- I guess it's not reading the config quite right. Is this running for you as-is?

I have a an additional thought. Should we have usersettings.json be part of the repo? If folks have their own settings there, they probably don't want them overwritten. Here's a suggestion: we move usersettings.json to usersettings.dist.json and in your settings class, let's load both and have the values in usersettings.json override those in usersettings.dist.json. If folks want to customize, we ask them to copy usersettings.dist.json to usersettings.json and make changes there (maybe just to the variables they want to change). This allows us to add new settings and defaults going forward without messing with customizations. Thoughts?

If this seems reasonable, in addition to making sure the code runs and tests pass, could you make this change?

[mattijsbliek]

Sorry about the test and the runtime, sloppy on my part. Both are fixed now :)

Like the idea of making it easy for folks to add their own user settings file 👍 I'm wondering if it would make more sense to go for an .env file instead? This would allow consumers more flexibility in whether to set them through a .env or other via other means. We could provide a .env.example that people can copy. I would probably avoid reading from that file though, and would just define config defaults in the config class itself.

This could also work with a usersettings.json btw, maybe an example file isn't even needed since the values are in the documentation. If we move the default values into the config file, we can get rid of usersettings.json completely, and just tell folks to create one and define overrides if needed.

No strong opinion about either of these solutions btw, so happy to implement what you think is best.

[ryanolf]

@mattijsbliek Thanks for the fixes. The code runs and works now so I could pull this in. I think we should resolve the usersettings.json issue first to minimize the hassle in upgrading. Right now, if someone has customized usersettings.json they won't be able to pull this update (or any updates) trivially and will have to do a merge, which some folks may not know how to do. If people happen to keep their old version of usersettings.json instead of doing a merge (e.g. git stash/pop), the program would crash with this update. So let's think this through and make sure we solve the updates issue.

I made a mistake in my initial release by version controlling a customizable file. I think the ideal path forward rectifies this with as little work by any current users as possible. To this end, ideally I'd remove usersettings.json from the repo but allow folks who modified it to to do a simple fast-forward merge, keeping their own custom version. It appears this isn't possible.

So here's what I propose. We remove usersettings.json from the repo and add it to .gitignore. If folks have unmodified usersettings.json (unlikely-- they probably have customized the room at least I'd think) the file will be deleted. If it's modified, they will get a message that they have an unclean working directory on any pull and be advised to stash by git. In case people do that, I'd like to make sure that works, which means we should continue to honor the settings in usersettings.json at least for now. Maybe we "deprecate" it and add a log message encouraging people to change to using the new configuration.

I like the suggestion of having settings like the ones we have so far in a .env file, and I like the idea of providing a sample with comments. We can update the readme to show an example of how to set the room variable (which most people will want to do) via either .env or a command line variable.

As far as where to put defaults, I lean toward having defaults in a JSON separate from the config class. This would make loading usersettings.json pretty trivial. First load the default JSON. Then Object.assign the usersettings.json to override defaults. Then overwrite with relevant values from process.env. I don't feel strongly here, though. Having defaults in config.js could also be OK.

It seems like dotenv is the canonical package for loading .env files. Since we use import instead of require it seems like we need to either do something like this or import dotenv/config directly. I think having the separate file (or maybe config.js?) load the module kinda makes sense.

Is this something you can work on and add to this PR?

[ryanolf]

@mattijsbliek If you give me write access to this branch I may be able to help draft the readme or something.