[image_picker] Fix typos in error messages for android #10188
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a typo in an error message across three different files. Thank you for improving the user-facing strings!
I've added a few comments regarding code duplication for the error message that this change highlights. Consolidating the error message into a single constant would improve the maintainability of the code. This is a suggestion for improvement and not a blocker for this PR.
|
|
||
| if (limit != null && limit < 2) { | ||
| throw ArgumentError.value(limit, 'limit', 'cannot be lower then 2'); | ||
| throw ArgumentError.value(limit, 'limit', 'cannot be lower than 2'); |
There was a problem hiding this comment.
This is another location where the duplicated error message is fixed. To improve long-term maintainability, it would be best to use a shared constant for this message. Please see my comment in packages/image_picker/image_picker_platform_interface/lib/src/types/multi_image_picker_options.dart for a more detailed suggestion.
|
|
||
| if (limit != null && limit < 2) { | ||
| throw ArgumentError.value(limit, 'limit', 'cannot be lower then 2'); | ||
| throw ArgumentError.value(limit, 'limit', 'cannot be lower than 2'); |
There was a problem hiding this comment.
This is another instance of the duplicated error message. To improve maintainability, please consider using a shared constant. I've left a more detailed comment on this topic in packages/image_picker/image_picker_platform_interface/lib/src/types/multi_image_picker_options.dart.
| static void _validate({int? limit}) { | ||
| if (limit != null && limit < 2) { | ||
| throw ArgumentError.value(limit, 'limit', 'cannot be lower then 2'); | ||
| throw ArgumentError.value(limit, 'limit', 'cannot be lower than 2'); |
There was a problem hiding this comment.
While fixing the typo is correct, this change highlights that the error message string is duplicated in multiple files (multi_image_picker_options.dart, media_options.dart, and image_picker_android.dart).
To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, it's better to define this string as a constant in one place and reuse it. This prevents future inconsistencies where a fix might be missed in one of the locations.
A possible approach:
- Define a public constant in a suitable location within the
image_picker_platform_interfacepackage, for example, at the top level of this file:const String kLimitLowerThanTwoError = 'cannot be lower than 2';
- Use this constant
kLimitLowerThanTwoErrorin all three files where this error is thrown.
|
This change is not test exempt; messages thrown outside of the package are part of the public surface, and this is a bug in that surface. There are already tests of the cases that throws these errors; they should be augmented to check that the message is correct (like this). |
|
The Linux repo_checks message says, "Dart changes are not allowed to other packages in image_picker in the same PR as changes to public Dart code in image_picker_platform_interface." Should I split the current PR into multiple ones? |
Yes, it will need to be two PRs; we don't allow bypassing that check, for safety reasons. |
| throwsA( | ||
| isA<ArgumentError>() | ||
| .having((ArgumentError error) => error.name, 'name', 'limit') | ||
| .having( | ||
| (ArgumentError error) => error.message, | ||
| 'message', | ||
| 'cannot be lower than 2', | ||
| ), | ||
| ), |
There was a problem hiding this comment.
I wasn't aware of this before I just checked, but these "throwsA" calls can be stored in a variable, so you don't need to repeat them multiple times in every file.
I'm not sure what the design rules are for that though. Maybe @stuartmorgan-g knows?
There was a problem hiding this comment.
I've stored the duplicate code in a local variable, and I'm happy to make changes if there's a better way to handle it.
hgraceb
changed the title
|
I split the code outside of the Android and iOS module into #10211. |
hgraceb
changed the title
hgraceb
changed the title
4 participants
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3