Skip to content

Conversation

@wayfarer3130
Copy link

Add hierarchical categories in loglevel v2

@wayfarer3130 wayfarer3130 changed the base branch from main to v2 March 4, 2025 14:14
@wayfarer3130 wayfarer3130 force-pushed the feat/v2-hierarchical-categories branch from 7a8e05b to 6bac8ea Compare March 4, 2025 14:25
lib/loglevel.js Outdated
return defaultLevel;
} else {
return inheritedLevel;
if (userLevel !== null) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes a bug where user level 0 wasn't applying.

lib/loglevel.js Outdated
* @type {number}
*/
var inheritedLevel;
var inheritedLevel = null;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These null changes fix a bug causing the levels to not apply correctly.

@wayfarer3130
Copy link
Author

@pimterry - I added the hierarchical loggers and the getLogger("parent").getLogger("child") === getLogger("parent","child")
with the inheritance of the default log level.

Copy link
Owner

@pimterry pimterry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added quite a few comments. This is a great start but there's a few ways we can simplify this. I've tried to cover the main points here so you get an idea - the main thing is that I don't think we need also the extra fields and edge cases.

Copy link
Contributor

@Mr0grog Mr0grog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t want to step on @pimterry’s toes in this review — these are just a few minor notes about syntax and public/private stuff.

@wayfarer3130 wayfarer3130 requested a review from pimterry March 11, 2025 00:56
@wayfarer3130
Copy link
Author

@pimterry @Mr0grog
I've updated this to address the comments.

  • Use rebuild to update both the method and inherited level
  • getLogger() returns the logger itself, at whatever level that happens to be, that is, log.getLogger()===log
  • Inherit methodFactory from parent, and children can inherit from any log instance after a rebuild
  • No manual rebuild is needed
  • No storage on symbols, but they can inherit from parent symbols
  • Single method for getLogger for both default and child loggers.

It needs docs updated, but I'm not going to complete that until the code is approved, then I will change just the docs.

Copy link
Owner

@pimterry pimterry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @wayfarer3130! I like the approach. A couple more comments but just on small details now, I think this is nearly there.

There are other details we could tweak, but we can fix those up later (I agree the indentation is currently a mess for example, we should standardize on something there - personally I'd lean to 4 spaces everywhere but whatever people prefer).

This is mergeable though imo once the two loggers API details here are covered and the exports changes are pulled out.

@wayfarer3130
Copy link
Author

Made the changes you requested

@wayfarer3130 wayfarer3130 requested a review from pimterry March 20, 2025 14:14
@pimterry pimterry merged commit ba7fc4d into pimterry:v2 Mar 20, 2025
10 checks passed
@pimterry
Copy link
Owner

Nice stuff, thanks @wayfarer3130! Merged 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants