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

Test-logger extension #432

Merged
merged 8 commits into from
Sep 11, 2023
Merged

Test-logger extension #432

merged 8 commits into from
Sep 11, 2023

Conversation

f3l1x98
Copy link
Contributor

@f3l1x98 f3l1x98 commented Aug 28, 2023

This adds some additional functionality to the test-logger as discussed #430 (comment)

Copy link
Member

@georg-schwarz georg-schwarz left a comment

Choose a reason for hiding this comment

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

Can you pls check if we can implement it by subclassing DefaultLogger instead to avoid duplicate logic?

@f3l1x98
Copy link
Contributor Author

f3l1x98 commented Aug 30, 2023

Can you pls check if we can implement it by subclassing DefaultLogger instead to avoid duplicate logic?

I intended to (that is why it is still in WIP, after all subclassing DefaultLogger will require some visibility changes in the logger)

@georg-schwarz
Copy link
Member

Ah wait, that will cause the dependency cycle again, right? Maybe we can move the default logger somewhere else?

@f3l1x98
Copy link
Contributor Author

f3l1x98 commented Sep 4, 2023

Ah wait, that will cause the dependency cycle again, right? Maybe we can move the default logger somewhere else?

Yes you are right.
I have moved the default-logger.ts into execution/logging.

@f3l1x98 f3l1x98 changed the title [WIP] Test-logger extension Test-logger extension Sep 4, 2023
Copy link
Member

@georg-schwarz georg-schwarz left a comment

Choose a reason for hiding this comment

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

LGTM.

I have another use-case in mind: a program using Jayvee might want to extract the logs (instead of reading the std out).

Could you move it from the test to the regular library.

Another suggestion: store all logs in a common string array of a wrapper that lets you filter the different log levels. This way the order of log messages is preserved between the different log levels.

@f3l1x98
Copy link
Contributor Author

f3l1x98 commented Sep 11, 2023

I could do that, but would do so in a separate PR given that this is another feature.

@f3l1x98 f3l1x98 merged commit 8e84faf into main Sep 11, 2023
2 checks passed
@f3l1x98 f3l1x98 deleted the feature/test-logger-cache branch September 11, 2023 10:30
@github-actions github-actions bot locked and limited conversation to collaborators Sep 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants