-
Notifications
You must be signed in to change notification settings - Fork 96
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
SdkBundle: Refactor Configuration #57
Conversation
src/Symfony/OtelSdkBundle/DependencyInjection/Configuration/TraceConfiguration.php
Show resolved
Hide resolved
...undle/DependencyInjection/Configuration/Validator/CustomClassImplementationValidatorTest.php
Show resolved
Hide resolved
@@ -22,17 +24,17 @@ class ExporterDsnParserTest extends TestCase | |||
'password' => 'secret', | |||
]; | |||
|
|||
public function testParseFullDsn() | |||
public function testParseFullDsn(): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we missing a cs-fixer rule to enforce snake_case ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this has never been done in the contrib repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm - a couple of questions and suggestions that can be actioned or ignored.
We might need some psalm ignores too, it's not happy.
I'm working on this. But it will be a bit more than ignores. |
if (!$returnType instanceof ReflectionNamedType || $returnType->getName() !== self::TYPE_BOOL) { | ||
throw new InvalidArgumentException( | ||
sprintf( | ||
'All provided closures must have boolean return type. Give: "%s"', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a typo: "Give" -> "Given"
}); | ||
} | ||
|
||
public static function createExceptionTrigger(array $requiredAttributes): Closure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two still need to be separated into dedicated Closure factories with a simple create methods as in the other one.
...fony/OtelSdkBundle/DependencyInjection/Configuration/Normalizer/ExporterConfigNormalizer.php
Show resolved
Hide resolved
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This issue has been automatically closed because it has not had recent activity, but it can be reopened. Thank you for your contributions. |
closes: open-telemetry/opentelemetry-php#652