-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[flutter_local_notifications] Throw ArgumentError when trying to initialize LocalNotification without per Platform settings #1559
[flutter_local_notifications] Throw ArgumentError when trying to initialize LocalNotification without per Platform settings #1559
Conversation
…ons without per Platform settings add: tests
The solution is not using asserts, because they are not flexible enough and also they force us to not use const constructors, which is undesirable. Throwing and ArgumentError is the right thing to do. The check will happen before the null check operator used on null value exception throws. Added more Readme and inline docs for the devs to make clearer. |
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.
Thanks for the PR. I left some comments on the changes but also wanted to ask if you could clarify the following point
The solution is not using asserts, because they are not flexible enough and also they force us to not use const constructors, which is undesirable.
Whilst I don't have an issue with using ArgumentError
, the point of const constructors doesn't apply to this PR. I assume you're referring to have code in the body of the constructor, if so wouldn't that happen for ArgumentError
s too? In actual fact, assertions are the one that do allow for const constructors. There a lot widgets within Flutter with const constructors and assertions. Perhaps you're not familiar with how this can be done but there's also an example within the repository
]) : assert(hour >= 0 && hour < 24), |
flutter_local_notifications/lib/src/flutter_local_notifications_plugin.dart
Outdated
Show resolved
Hide resolved
flutter_local_notifications/lib/src/flutter_local_notifications_plugin.dart
Outdated
Show resolved
Hide resolved
flutter_local_notifications/example/integration_test/flutter_local_notifications_test.dart
Show resolved
Hide resolved
yes, but they do not refer to Platform. |
Not sure what you were trying to answer with this one but if it's to do with assertions then given my comments on the PR where the platform checks you added aren't needed as they already exist, suppose assertions were added then they would be done within the various if blocks for each platform to check to see if the corresponding settings isn't null. |
add: new line in the readme change: the ArgumentError texts change: fix: capitalization of macOs change: apply text changes in the integration test change: make test case text more obvious
Thanks for making the changes. Been really busy so haven't been able to revisit this until now and I just noticed a minor formatting thing for API docs so I made the change directly here so this can go out soon |
change: throw ArgumentError when trying to initialize LocalNotification without per Platform settings
add: tests
add: inline docs