Fix issues with Logging module ('kolibri.lib.logging')

[iskipu]

This PR fixes both the issues mentioned in #11819 and also it fixes #11821 (the reason why the modal is not closing is because the logging module's .log method was not working in the below mentioned lines in the code base

kolibri/packages/kolibri-common/components/MeteredConnectionNotificationModal.vue

Lines 93 to 94 in 1123395

	logging.log('mounted', isMetered);
	logging.log(data);

)

I hope this changes solves the issue.

Fixes #11819, fixes #11821

Results:

Image (after making .log function work)

Image (after making .setLevel function work)

[github-actions]

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.16.0b13.dev0_git.11.gef7260a3.pex
Windows Installer (EXE)	kolibri-0.16.0b13.dev0+git.11.gef7260a3-unsigned.exe
Debian Package	kolibri_0.16.0b13.dev0+git.11.gef7260a3-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.16.0b13.dev0+git.11.gef7260a3-0.3.0.dmg
Android Package (APK)	kolibri-0.16.0b13.dev0+git.11.gef7260a3-0.1.0-debug.apk
TAR file	kolibri-0.16.0b13.dev0+git.11.gef7260a3.tar.gz
WHL file	kolibri-0.16.0b13.dev0+git.11.gef7260a3-py2.py3-none-any.whl

[rtibbles]

The logging module isn't broken. The call should be info not log. Please update the broken reference instead.

[iskipu]

The logging module isn't broken. The call should be info not log. Please update the broken reference instead.

Hi @rtibbles since logging.js is a wrapper around npm module loglevel i thought it would be better if it directly supports .log function just like loglevel as (mentioned here), Plz take a look at it and if u think its not necessary i will just make changes to the reference directly.

and also any comments/suggestions regarding .setLevel function ?

[rtibbles]

It's there as an alias for debug, so if the intention is to debug then debug should be used (although this seems unintuitive because console.log is functionally more equivalent to the info method), so I don't want to expose this.

[rtibbles]

I haven't looked closely at the setLevel issue. If it's not working that's a bug in loglevel.

[iskipu]

It's there as an alias for debug, so if the intention is to debug then debug should be used (although this seems unintuitive because console.log is functionally more equivalent to the info method), so I don't want to expose this.

Ohk Understood, i changed the .log to .info in MeteredConnectionNotificationModal.vue like u suggested this closes #11821

[iskipu]

I haven't looked closely at the setLevel issue. If it's not working that's a bug in loglevel.

I did check, this its not a bug in loglevel
Headsup: my explanation might be very bad 😅

the problem is that in loglevel module .debug, .info, .error ... this functions get generated dynamically based on the value of level set by setLevel (refer this link)

and in our code base the following line is defined which directy uses the function (lets say x) at that point (i.e when setLevel is 3 (default value)) but since as i mentioned earlier that the functions are generated dynamically we are stuck with x even after changing the level with the help of setLevel.

kolibri/kolibri/core/assets/src/logging.js

Line 18 in 171f6bb

this[name] = logFunction.bind(console, this.messagePrefix(name));

Inorder to fix this i proposed this changes

      this[name] = param => {
          return this.logger[name].bind(console, this.messagePrefix(name))(param);
        };

Sorry if my explanation is bad i hope it made some sense let me know if i am doing something wrong.

[rtibbles]

Yes, you're right - it seems since this was first implemented, loglevel has implemented a plugin interface, with a specific example of adding a prefix to log statements.

I think the best thing to do would be to use this example to update the code, rather than binding to the global console!

https://npmjs.com/package/loglevel#writing-plugins

[iskipu]

I think the best thing to do would be to use this example to update the code, rather than binding to the global console!

Hi @rtibbles great idea, made a new commit with ur suggestion.

[rtibbles]

This looks good to go, thanks @iskipu - I am going to defer merging this until after we have released 0.16.0, as the metered connection modal is not currently active in any of our products, and we are only merging critical bug fixes at this time. We should be able to merge this in a few weeks!