Fix parameter_assignments documentation for null safety and updated dart syntax #62191
Conversation
|
cc @parlough |
|
Thank you for your contribution! This project uses Gerrit for code reviews. Your pull request has automatically been converted into a code review at: https://dart-review.googlesource.com/c/sdk/+/466600 Please wait for a developer to review your code review at the above link; you can speed up the review if you sign into Gerrit and manually add a reviewer that has recently worked on the relevant code. See CONTRIBUTING.md to learn how to upload changes to Gerrit directly. Additional commits pushed to this PR will update both the PR and the corresponding Gerrit CL. After the review is complete on the CL, your reviewer will merge the CL (automatically closing this PR). |
parlough
left a comment
parlough
left a comment
There was a problem hiding this comment.
Thanks for updating this here!
Do note the rest of the review will be in https://dart-review.googlesource.com/c/sdk/+/466600
Co-authored-by: Parker Lougheed <parlough@gmail.com>
|
https://dart-review.googlesource.com/c/sdk/+/466600 has been updated with the latest commits from this pull request. |
1 similar comment
|
https://dart-review.googlesource.com/c/sdk/+/466600 has been updated with the latest commits from this pull request. |
|
https://dart-review.googlesource.com/c/sdk/+/466600 has been updated with the latest commits from this pull request. |
1 similar comment
|
https://dart-review.googlesource.com/c/sdk/+/466600 has been updated with the latest commits from this pull request. |
| Assigning new values to parameters is generally a bad practice unless an | ||
| operator such as `??=` is used. Otherwise, arbitrarily reassigning parameters | ||
| is usually a mistake. | ||
| Assigning new values to parameters is generally a bad practice, even when using |
There was a problem hiding this comment.
Drive-by comment: I don't think the text should be changed here. The lint actually does not emit a diagnostic when it encounters myParameter ??= someNonNullValue;. The rationale is that this particular kind of assignment is appropriate. It is essentially an emulation of non-constant default values. For example:
var nonConstantDefault = 0;
void f([int? i]) {
i ??= ++nonConstantDefault; // OK.
i = 42; // LINT.
}
| operator such as `??=` is used. Otherwise, arbitrarily reassigning parameters | ||
| is usually a mistake. | ||
| Assigning new values to parameters is generally a bad practice, even when using | ||
| operators such as `??=`. Arbitrarily reassigning parameters is usually a |
There was a problem hiding this comment.
With that, it makes sense to keep saying 'Otherwise, ...' as well.
| void badFunction(int parameter) { // LINT | ||
| parameter = 4; |
There was a problem hiding this comment.
The comment // LINT should probably be placed on the line that actually gives rise to the diagnostic message:
| void badFunction(int parameter) { | |
| parameter = 4; // LINT |
Similarly for the other examples.
| ```dart | ||
| void badFunction(int required, {int optional: 42}) { // LINT | ||
| void badFunction(int required, {int? optional}) { // LINT | ||
| optional ??= 8; |
There was a problem hiding this comment.
This one does not give rise to a lint message, it is an example of the "GOOD" use of ??=. The following could be used as a "BAD" example:
void badFunction(int? nonOptional) {
nonOptional ??= 8; // LINT
}
| ```dart | ||
| void badFunctionPositional(int required, [int optional = 42]) { // LINT | ||
| void badFunctionPositional(int required, [int? optional]) { // LINT | ||
| optional ??= 8; |
There was a problem hiding this comment.
This should again be relabeled as a "GOOD" example, no lint is reported.
| void actuallyGood(int required, {int optional}) { // OK | ||
| optional ??= ...; | ||
| void actuallyGood(int required, {int? optional}) { // OK | ||
| final value = optional ?? 8; |
There was a problem hiding this comment.
In order to show that ??= is accepted in this case (hence the name actuallyGood), this example should keep the assignment:
| final value = optional ?? 8; | |
| optional ??= 8; |
An example using a fresh local variable would be fine, too.
| void actuallyGoodPositional(int required, [int optional]) { // OK | ||
| optional ??= ...; | ||
| void actuallyGoodPositional(int required, [int? optional]) { // OK | ||
| final value = optional ?? 8; |
There was a problem hiding this comment.
The situation is similar here:
| final value = optional ?? 8; | |
| optional ??= 8; |
3 participants
Fix 7048