cr3ative / homebridge-apcaccess

what if apcaccess, but in homebridge??
MIT License
8 stars 5 forks source link

Only log when state changes #16

Closed davidjbradshaw closed 1 year ago

davidjbradshaw commented 1 year ago

This PR changes the logging so that it only output state, when the state has actually changed since the last time it was output. I have also changed the main code to use log.error for error messages.

I think this is a better way of addressing #14, as now only useful messages get logged, rather than have an all or nothing approach.

cr3ative commented 1 year ago

Brilliant, thanks for this. I'll combine the two PRs and get a new version out soon.

cr3ative commented 1 year ago

I've merged this change set in to #15: https://github.com/cr3ative/homebridge-apcaccess/pull/15/commits/15af44b747232d934431d25b363aec17f84dc1e4

Can you take a look to ensure I got the spirit of that correct?

davidjbradshaw commented 1 year ago

Given that it now logs very little, I would make the default option to have logging on, rather than off. For most HomeBridge projects the default log option is minimal rather than off. Doing it this way means new uses can see it working without any effort, but after start up almost nothing gets logged.

It might be better to keep it simple and just not to have the option at all.

davidjbradshaw commented 1 year ago

Also I think the way you have it now will just blow up if you turn logging off, as you now need to mock the info, warn, error and update methods of this.log.

cr3ative commented 1 year ago

Also I think the way you have it now will just blow up if you turn logging off, as you now need to mock the info, warn, error and update methods of this.log.

You're right, of course. That's what I get for coding without testing it. I'll rework this and default to minimal.

davidjbradshaw commented 1 year ago

If you want an off option still I would suggest

const T = () => true
const logOff = (log) => ({
   ...log,
   info: T,
   update: T,
})

So you still log errors.

cr3ative commented 1 year ago

Thanks, that logOff tip was dead helpful. Rolled a variant of this in to #15, so let's check things out over there.