Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve category types #21

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

KROT47
Copy link

@KROT47 KROT47 commented Oct 9, 2024

No description provided.

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.24%. Comparing base (988a27a) to head (dafe3a9).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #21      +/-   ##
==========================================
- Coverage   97.24%   97.24%   -0.01%     
==========================================
  Files          10       10              
  Lines        1018     1016       -2     
  Branches      217      217              
==========================================
- Hits          990      988       -2     
  Misses         28       28              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

getChild(
subcategory: string | readonly [string] | readonly [string, ...string[]],
): Logger;
getChild(subcategory: Category): Logger;
Copy link
Owner

Choose a reason for hiding this comment

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

We should not allow empty arrays here.

Copy link
Author

Choose a reason for hiding this comment

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

why?
I have a case where I generate category dynamically and it can be empty. In the case of this lib there is no real difference between returning current logger from getChild or returning a child as new instance. We just need to get the correct logger.

Copy link
Author

Choose a reason for hiding this comment

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

In my fork I even made that if a category is empty string, empty array, null or undefined then it will return current logger instead of new child
Seems ok by me)

Copy link
Owner

Choose a reason for hiding this comment

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

I think it's misleading that the method name is getChild() but the object returned is clearly not a child logger, and I don't want the signature of the method to change. 🤔

Comment on lines 431 to 430
private constructor(parent: LoggerImpl | null, category: readonly string[]) {
constructor(parent: LoggerImpl | null, category: CategoryList) {
Copy link
Owner

Choose a reason for hiding this comment

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

Why did you remove the private keyword?

Copy link
Author

Choose a reason for hiding this comment

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

was playing with class inheritance. this can be rolled back

Copy link
Author

Choose a reason for hiding this comment

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

done

| string
| readonly [string]
| readonly [string, ...(readonly string[])],
subcategory: Category,
Copy link
Owner

Choose a reason for hiding this comment

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

We should not allow empty arrays here.

Copy link
Author

Choose a reason for hiding this comment

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

same as above

@@ -692,7 +686,7 @@ export class LoggerCtx implements Logger {
}

getChild(
subcategory: string | readonly [string] | readonly [string, ...string[]],
subcategory: Category,
Copy link
Owner

Choose a reason for hiding this comment

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

We should not allow empty arrays here.

Copy link
Author

Choose a reason for hiding this comment

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

same as above

Comment on lines 1 to 3
export type CategoryList = Readonly<string[]>;

export type Category = string | CategoryList;
Copy link
Owner

Choose a reason for hiding this comment

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

Could you add TSDoc comments for those two types?

Copy link
Author

Choose a reason for hiding this comment

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

done

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.

2 participants