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

#70 - implemented new AbstractFactory for PsrLoggerAdapter #99

Conversation

@dennybrandes
Copy link

dennybrandes commented Aug 25, 2019

Add feature described in #70

  • Are you creating a new feature?

    • Add documentation for the new feature.
    • Add a CHANGELOG.md entry for the new feature.
  • Is this related to documentation?

michalbundyra and others added 3 commits Aug 23, 2019
@dennybrandes

This comment has been minimized.

Copy link
Author

dennybrandes commented Aug 25, 2019

Documentation is missing. Any suggestion where to place it exactly. There is a part ServiceManager and one for PSR-3. In which of them could we put it in?

@dennybrandes dennybrandes marked this pull request as ready for review Aug 25, 2019
@dennybrandes dennybrandes changed the base branch from master to develop Aug 28, 2019
@weierophinney

This comment has been minimized.

Copy link
Member

weierophinney commented Dec 2, 2019

@dennybrandes Put the documentation under the ServiceManager section, but reference and link to it from the PSR-3 section.

@michalbundyra michalbundyra added this to the 2.12.0 milestone Dec 2, 2019
@dennybrandes

This comment has been minimized.

Copy link
Author

dennybrandes commented Dec 4, 2019

@weierophinney I added documentation now.

Copy link
Member

froschdesign left a comment

First: Thanks for adding the documentation! 👍

I added comments with some suggestions for improvement.

@@ -61,3 +61,6 @@ $logger->addProcessor(new Zend\Log\Processor\PsrPlaceholder);
$logger->info('User with email {email} registered', ['email' => 'user@example.org']);
// logs message 'User with email user@example.org registered'
```
## Usage with ServiceManager

For usage with ServiceManager, check this [PsrLoggerAbstractServiceFactory](./service-manager.md#PsrLoggerAbstractServiceFactory).

This comment has been minimized.

Copy link
@froschdesign

froschdesign Dec 4, 2019

Member
  • Use the full name for the service manager here: "zend-servicemanager" to make clear what is meant.
  • All class names must be set in backticks: `PsrLoggerAbstractServiceFactory`
  • Instead of "check this" we should use a formulation like "see the Service Manager Integration section" and set the link on "Service Manager Integration section".
  • For linking to a page in the same the directory it is enough to use the filename without any dots or slashes. (see MkDocs documentation)
@@ -110,6 +110,23 @@ Because the main filter is `Priority`, it can be set directly too:
];
```

## PsrLoggerAbstractServiceFactory

Same as above, you can use PsrLoggerAbstractServiceFactory to create a PSR-3 conform logger.

This comment has been minimized.

Copy link
@froschdesign

froschdesign Dec 4, 2019

Member
  • This is a different factory and we should add separate description. "Same as above" should be replaced.
  • All class names must be set in backticks: PsrLoggerAbstractServiceFactory
  • A link on "PSR-3 logger" with the target to "PSR-3 Support" page would be good here.
## PsrLoggerAbstractServiceFactory

Same as above, you can use PsrLoggerAbstractServiceFactory to create a PSR-3 conform logger.
Just use following configuration instead.

This comment has been minimized.

Copy link
@froschdesign

froschdesign Dec 4, 2019

Member

"just", "simple", "easy" etc. should be avoided in a (technical) documentation. It's hard to say if it's really easy or simple for everyone.

return [
'service_manager' => [
'abstract_factories' => [
'Zend\Log\PsrLoggerAbstractServiceFactory',

This comment has been minimized.

Copy link
@froschdesign

froschdesign Dec 4, 2019

Member

Please use the class name instead of the string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.