[M] Change in dart/sdk[main]: [linter, DAS] Makes `use_null_aware_elements` to report on cascade el...
Felipe Morschel (Gerrit)
Felipe Morschel has uploaded the change for review![]( "Open in Gerrit")
Commit message
[linter, DAS] Makes `use_null_aware_elements` to report on cascade elements
Change information
- M pkg/analysis_server/lib/src/services/correction/dart/convert_null_check_to_null_aware_element_or_entry.dart
- M pkg/analysis_server/test/src/services/correction/fix/convert_null_check_to_null_aware_element_or_entry_test.dart
- M pkg/linter/lib/src/rules/use_null_aware_elements.dart
- M pkg/linter/test/rules/use_null_aware_elements_test.dart
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted and added 1 comment![]( "Open in Gerrit")
Votes added by Samuel Rawlins
| Code-Review | +1 |
1 comment
Related details
- Brian Wilkerson
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Brian Wilkerson
| Code-Review | +1 |
1 comment
Future<void> test_cascade_case_getter_list() async {I think you mean "spread" rather than "cascade".
Also, it would be good to have at least one test involving a set. It probably doesn't need to be covered as completely as lists and maps, but it should be tested.
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
Future<void> test_cascade_case_getter_list() async {I think you mean "spread" rather than "cascade".
Also, it would be good to have at least one test involving a set. It probably doesn't need to be covered as completely as lists and maps, but it should be tested.
- Brian Wilkerson
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted Code-Review+1![]( "Open in Gerrit")
- Brian Wilkerson
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Brian Wilkerson
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
I'm not familiar with anyone from `dev_compilers`, so if you need to reassign @elliot...@google.com please do so. This is a really simple change to land a new case for the `use_null_aware_elements` lint.
Related details
- Brian Wilkerson
- Elliott Brooks
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson added 1 comment![]( "Open in Gerrit")
if (types && typeArguments != null) ...typeArguments,Should we be recommending replacing this with
```
if (types) ...?typeArguments
```
Related details
- Felipe Morschel
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel added 1 comment![]( "Open in Gerrit")
if (types && typeArguments != null) ...typeArguments,Should we be recommending replacing this with
```
if (types) ...?typeArguments
```
We can. But we probably need a good definition of what cases we handle. For example, what if we are doing something like:
```dart
if (x case var y when y != null) y
```
This has the same meaning as `x case var y?` but we currently don't report it either.
Related details
- Brian Wilkerson
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Brian Wilkerson
| Code-Review | +1 |
1 comment
if (types && typeArguments != null) ...typeArguments,Felipe MorschelShould we be recommending replacing this with
```
if (types) ...?typeArguments
```
We can. But we probably need a good definition of what cases we handle. For example, what if we are doing something like:
```dart
if (x case var y when y != null) y
```This has the same meaning as `x case var y?` but we currently don't report it either.
We're never going to flag every possible equivalent code structure. I'm not sure we should flag the one I noticed. The goal is to flag the most common cases so that we bring the most value for the lowest cost. An 80 / 20 rule: cover the 20% of the code structures that catch 80% of the places that could be improved. We might already be there. I don't know whether this case makes the cut, but thought it would be worth asking. I'm fine with you not changing it.
Related details
- Felipe Morschel
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel added 1 comment![]( "Open in Gerrit")
if (types && typeArguments != null) ...typeArguments,Felipe MorschelShould we be recommending replacing this with
```
if (types) ...?typeArguments
```
Brian WilkersonWe can. But we probably need a good definition of what cases we handle. For example, what if we are doing something like:
```dart
if (x case var y when y != null) y
```This has the same meaning as `x case var y?` but we currently don't report it either.
We're never going to flag every possible equivalent code structure. I'm not sure we should flag the one I noticed. The goal is to flag the most common cases so that we bring the most value for the lowest cost. An 80 / 20 rule: cover the 20% of the code structures that catch 80% of the places that could be improved. We might already be there. I don't know whether this case makes the cut, but thought it would be worth asking. I'm fine with you not changing it.
I'd much rather we do the one I hinted on the comment above. Since that can be transformed to a single null-aware element.
But I still think that is maybe a different lint job (to simplify that expression) so we get back here to the expressions we know how to handle. So I'll mark this as resolved.
Thanks for the suggestion!
Related details
- Brian Wilkerson
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |