-
Notifications
You must be signed in to change notification settings - Fork 184
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
Fix: Inject defaults in service provider #921
Fix: Inject defaults in service provider #921
Conversation
@@ -22,10 +22,48 @@ | |||
public function register(Container $app) | |||
{ | |||
$app[TalkHelper::class] = function ($app) { | |||
$categories = $app->config('talk.categories'); | |||
|
|||
if ($categories === null) { |
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.
What about something like:
$categories = $app->config('talk.categories') ?: [
'api' => 'APIs (REST, SOAP, etc.)',
'continuousdelivery' => 'Continuous Delivery',
// etc.
];
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.
Could do, but it's just not very readable, so personally I avoid ternary statements like that.
$categories = $app->config('talk.categories'); | ||
|
||
if ($categories === null) { | ||
$categories = [ |
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.
Not even sure if we need any of this functionality, since all of these are defined in
although with different values.
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 assume this is for displaying default values in case they aren't set in the config file.
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, it is, but why would they not be set?
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.
When it was introduced in this commit: f3b9384, i assume it was so people would have an easier time upgrading.
By now i think it should throw an error if it is not properly set.This would make sure people wouldn't launch their CFP with the wrong categories/types etc
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.
@@ -22,10 +22,48 @@ | |||
public function register(Container $app) | |||
{ | |||
$app[TalkHelper::class] = function ($app) { | |||
$categories = $app->config('talk.categories'); | |||
|
|||
if ($categories === null) { |
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.
Could do, but it's just not very readable, so personally I avoid ternary statements like that.
This PR
TalkHelper
handle defaults