-
Notifications
You must be signed in to change notification settings - Fork 17
Feature add warning / exception dependning on Gotenberg version #176
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
base: 1.x
Are you sure you want to change the base?
Conversation
bbd9815
to
3bfbb0d
Compare
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.
Great job 🤩
/** | ||
* @param '>'|'<'|'>='|'<='|'=' $operator | ||
*/ | ||
protected function warningIf(string $operator, string|Version $version, string|\Stringable $message): 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.
protected function warningIf(string $operator, string|Version $version, string|\Stringable $message): void | |
protected function warnIf(string $operator, string|Version $version, string|\Stringable $message): 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.
hmmmm... I feel warning is closer to the underlying logic.
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.
I propose to rename this method to describe the action. It can be logWarningIf
or just logIf
. (but I prefer warnIf
😛 )
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.
renamed
/** | ||
* @param '>'|'<'|'>='|'<='|'=' $operator | ||
*/ | ||
protected function warningIf(string $operator, string|Version $version, string|\Stringable $message): 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.
I propose to rename this method to describe the action. It can be logWarningIf
or just logIf
. (but I prefer warnIf
😛 )
/** | ||
* @param '>'|'<'|'>='|'<='|'=' $operator | ||
*/ | ||
protected function logWarningIfVersionIs(string $operator, string|Version $version, string|\Stringable $message): 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.
Since most of calls are done with a <
operator, what about naming it introducedIn
like in AbstractBuilder
?
Description