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

feat: Logger #10

Merged
merged 2 commits into from
May 24, 2024
Merged

feat: Logger #10

merged 2 commits into from
May 24, 2024

Conversation

przemyslaw-przylucki
Copy link
Contributor

@przemyslaw-przylucki przemyslaw-przylucki commented May 17, 2024

Adds basic support for logs.

Needs some cleanup but would appreciate any feedback :)

@przemyslaw-przylucki przemyslaw-przylucki marked this pull request as draft May 17, 2024 22:00
@przemyslaw-przylucki przemyslaw-przylucki changed the title feat: Log handling feat: Logger May 17, 2024
@brendt
Copy link
Member

brendt commented May 22, 2024

Seems good!

Would you be up to writing a short documentation page of how logging can be used as well? It could be as a comment on this PR, but it would help me grasp the implications for end-users, and not just judge this PR from a technical POV :)

@coveralls
Copy link

Pull Request Test Coverage Report for Build 9134732003

Details

  • 43 of 58 (74.14%) changed or added relevant lines in 7 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.5%) to 88.303%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Discovery/LogHandlerDiscovery.php 9 10 90.0%
src/Log/LoggerInitializer.php 0 4 0.0%
src/Log/GenericLogger.php 11 21 52.38%
Totals Coverage Status
Change from base Build 9061598786: -0.5%
Covered Lines: 1004
Relevant Lines: 1137

💛 - Coveralls

Copy link
Member

@brendt brendt left a comment

Choose a reason for hiding this comment

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

I'm actually going to merge this and make some changes myself, because I need this logger to work on some more stuff 😁 I left some comments so that you know what I intend to change, and we can further finetune it down the line :)

/** @var LogChannel $channel */
$channel = $this->container->get($channelName);

$config = $this->logConfig->channelsConfig[$channelName] ?? [];
Copy link
Member

Choose a reason for hiding this comment

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

This config should be an object with typed properties, not an array

*
* @return array<int, HandlerInterface>|HandlerInterface
*/
public function handler(Level $level, array $config): array|HandlerInterface;
Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be better to call this method getHandlers and always have it return an array, for simplicity. Same for getProcessors

/** @var LogChannel[] */
public array $channels = [],
/** @var array<class-string<LogChannel>, array|string> */
public array $channelsConfig = [],
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we need to improve channel config


$config = new LogConfig(
channelsConfig: [
AppendLogChannel::class => [
Copy link
Member

Choose a reason for hiding this comment

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

What if we had an array of channels, with instances of channels that we configured?

new LogConfig(
    channels: [
        new AppendLogChannel(path: $filePath)
    ],
)

That way we'd combine channel and channelConfig into one.

@brendt brendt marked this pull request as ready for review May 24, 2024 18:39
@brendt brendt merged commit d869a25 into tempestphp:main May 24, 2024
4 checks passed
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.

3 participants