[L] Change in dart/sdk[main]: Add a refactor to remove a name from a constructor
Brian Wilkerson (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danny Tuppeny (Gerrit)
Danny Tuppeny added 6 comments![]( "Open in Gerrit")
Future<ResponseMessage> sendExecuteCommand(I think this is unused now
Future<ResponseMessage> sendExecuteCommand(I think this is unused now
void f() {
C();
}I think this code probably isn't necessary in the failure cases?
C.name();Is there value in testing a tear-off too?
String? failureMessage,It doesn't look like anything passes this - should it? (or should we add a default `= "There's already a default constructor."`? Since this message us shown to the user, it might be worth validating.
Related details
- Brian Wilkerson
- 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 voted and added 6 comments![]( "Open in Gerrit")
Votes added by Brian Wilkerson
| Commit-Queue | +1 |
6 comments
I think this is unused now
Done
I think this is unused now
Done
I think this code probably isn't necessary in the failure cases?
Done
Is there value in testing a tear-off too?
Yes, thanks for catching that.
String? failureMessage,It doesn't look like anything passes this - should it? (or should we add a default `= "There's already a default constructor."`? Since this message us shown to the user, it might be worth validating.
Given that this is local to this test, I agree that we don't need to pass in a value. I've just hardcoded the expected value below.
Related details
- Danny Tuppeny
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danny Tuppeny (Gerrit)
Danny Tuppeny added 1 comment![]( "Open in Gerrit")
String? failureMessage,Brian WilkersonIt doesn't look like anything passes this - should it? (or should we add a default `= "There's already a default constructor."`? Since this message us shown to the user, it might be worth validating.
Given that this is local to this test, I agree that we don't need to pass in a value. I've just hardcoded the expected value below.
yeah, that makes more sense if there aren't other cases to test. Thanks!
Related details
- 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 and added 2 comments![]( "Open in Gerrit")
Votes added by Samuel Rawlins
| Code-Review | +1 |
2 comments
reason: "There's already a default constructor.",Should this say "unnamed constructor"? We don't know if there is a default constructor (an implicitly defined unnamed, zero-parameter constructor).
Related details
- Brian Wilkerson
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
| Commit-Queue | +2 |
1 comment
Should this say "unnamed constructor"? We don't know if there is a default constructor (an implicitly defined unnamed, zero-parameter constructor).
Yes, that's more accurate. Thanks and done.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Commit Queue submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
5 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: pkg/analysis_server/test/src/services/refactoring/remove_constructor_name_test.dart
Insertions: 1, Deletions: 1.
@@ -463,7 +463,7 @@
throwsA(
isResponseError(
ServerErrorCodes.refactoringComputeStatusFailure,
- message: "There's already a default constructor.",
+ message: "There's already an unnamed constructor.",
),
),
);
```
```
The name of the file: pkg/analysis_server/lib/src/services/refactoring/remove_constructor_name.dart
Insertions: 1, Deletions: 1.
@@ -43,7 +43,7 @@
for (var constructor in element.enclosingElement.constructors) {
if (constructor.name == 'new' || constructor.name == null) {
return ComputeStatusFailure(
- reason: "There's already a default constructor.",
+ reason: "There's already an unnamed constructor.",
);
}
}
```
Change information
Add a refactor to remove a name from a constructor
- M pkg/analysis_server/lib/src/services/refactoring/framework/refactoring_processor.dart
- A pkg/analysis_server/lib/src/services/refactoring/remove_constructor_name.dart
- A pkg/analysis_server/test/src/services/refactoring/remove_constructor_name_test.dart
- M pkg/analysis_server/test/src/services/refactoring/test_all.dart
Code-Review: +1 by Samuel Rawlins
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danny Tuppeny (Gerrit)
Danny Tuppeny added 1 comment![]( "Open in Gerrit")
reason: "There's already a default constructor.",Brian WilkersonShould this say "unnamed constructor"? We don't know if there is a default constructor (an implicitly defined unnamed, zero-parameter constructor).
Yes, that's more accurate. Thanks and done.
It might be useful to have a glossary somewhere describing some of these terms, because I've probably also used these interchangeably (I got the impression we were avoiding using "unnamed constructor" and I probably assumed "default" was an appropriate name for this and probably would've called the other one an "implicit constructor").
Related details
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")
reason: "There's already a default constructor.",Brian WilkersonShould this say "unnamed constructor"? We don't know if there is a default constructor (an implicitly defined unnamed, zero-parameter constructor).
Danny TuppenyYes, that's more accurate. Thanks and done.
It might be useful to have a glossary somewhere describing some of these terms, because I've probably also used these interchangeably (I got the impression we were avoiding using "unnamed constructor" and I probably assumed "default" was an appropriate name for this and probably would've called the other one an "implicit constructor").
We have a glossary (https://dart.dev/resources/glossary) but it doesn't contain either of these terms.
The diagnostic messages appear to be consistent in their use of 'unnamed' to mean a constructor that doesn't have a name, but are inconsistent with the use of 'default' to sometimes mean the synthetic constructor defined when there is no explicit constructor and sometimes to mean an unnamed zero parameter constructor.
I think we should be consistent with the spec, which uses 'default' for the synthetic constructor, 'unnamed' for a constructor that has no name, and doesn't have a term for a zero parameter constructor.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danny Tuppeny (Gerrit)
Danny Tuppeny added 1 comment![]( "Open in Gerrit")
reason: "There's already a default constructor.",Brian WilkersonShould this say "unnamed constructor"? We don't know if there is a default constructor (an implicitly defined unnamed, zero-parameter constructor).
Danny TuppenyYes, that's more accurate. Thanks and done.
Brian WilkersonIt might be useful to have a glossary somewhere describing some of these terms, because I've probably also used these interchangeably (I got the impression we were avoiding using "unnamed constructor" and I probably assumed "default" was an appropriate name for this and probably would've called the other one an "implicit constructor").
We have a glossary (https://dart.dev/resources/glossary) but it doesn't contain either of these terms.
The diagnostic messages appear to be consistent in their use of 'unnamed' to mean a constructor that doesn't have a name, but are inconsistent with the use of 'default' to sometimes mean the synthetic constructor defined when there is no explicit constructor and sometimes to mean an unnamed zero parameter constructor.
I think we should be consistent with the spec, which uses 'default' for the synthetic constructor, 'unnamed' for a constructor that has no name, and doesn't have a term for a zero parameter constructor.
Thanks! This all sounds reasonable to me. I thought we had previously said not to use unnamed. I searched my email to understand where that had come from and the only thing I found that was close to this was here:
https://github.com/dart-lang/sdk/issues/62393#issuecomment-3745407315
there's no such thing as an unnamed constructor
I think there might have been some discussions at the time around what the "name" was in a constructor (eg. in MyClass.foo()`), possibly because we'd been discussing navigation (or occurrences) for these things, so some of these things may have blurred together too.
Would it be useful to have these terms (and primary + secondary?) added to https://dart.dev/resources/glossary since if we use them in diagnostics/fixes, they're going to be used by users too?
Related details
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")
reason: "There's already a default constructor.",Brian WilkersonShould this say "unnamed constructor"? We don't know if there is a default constructor (an implicitly defined unnamed, zero-parameter constructor).
Danny TuppenyYes, that's more accurate. Thanks and done.
Brian WilkersonIt might be useful to have a glossary somewhere describing some of these terms, because I've probably also used these interchangeably (I got the impression we were avoiding using "unnamed constructor" and I probably assumed "default" was an appropriate name for this and probably would've called the other one an "implicit constructor").
Danny TuppenyWe have a glossary (https://dart.dev/resources/glossary) but it doesn't contain either of these terms.
The diagnostic messages appear to be consistent in their use of 'unnamed' to mean a constructor that doesn't have a name, but are inconsistent with the use of 'default' to sometimes mean the synthetic constructor defined when there is no explicit constructor and sometimes to mean an unnamed zero parameter constructor.
I think we should be consistent with the spec, which uses 'default' for the synthetic constructor, 'unnamed' for a constructor that has no name, and doesn't have a term for a zero parameter constructor.
Thanks! This all sounds reasonable to me. I thought we had previously said not to use unnamed. I searched my email to understand where that had come from and the only thing I found that was close to this was here:
https://github.com/dart-lang/sdk/issues/62393#issuecomment-3745407315
there's no such thing as an unnamed constructor
I think there might have been some discussions at the time around what the "name" was in a constructor (eg. in MyClass.foo()`), possibly because we'd been discussing navigation (or occurrences) for these things, so some of these things may have blurred together too.
Would it be useful to have these terms (and primary + secondary?) added to https://dart.dev/resources/glossary since if we use them in diagnostics/fixes, they're going to be used by users too?
... the only thing I found that was close to this was here ...
That is what the spec says (indirectly), but since then I've been leaning more toward using what I believe is the common meaning of the term and saying that an 'unnamed' constructor is a constructor whose name is the same as the class in which it's defined without a qualifying period and name.
Would it be useful to have these terms ...
I don't know. I think we only add entries for terms that might not be well-understood by our users, where "well-understood" is hard to quantify. It sounds like it would be a good question for the documentation team.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danny Tuppeny (Gerrit)
Danny Tuppeny added 1 comment![]( "Open in Gerrit")
reason: "There's already a default constructor.",Brian WilkersonShould this say "unnamed constructor"? We don't know if there is a default constructor (an implicitly defined unnamed, zero-parameter constructor).
Danny TuppenyYes, that's more accurate. Thanks and done.
Brian WilkersonIt might be useful to have a glossary somewhere describing some of these terms, because I've probably also used these interchangeably (I got the impression we were avoiding using "unnamed constructor" and I probably assumed "default" was an appropriate name for this and probably would've called the other one an "implicit constructor").
Danny TuppenyWe have a glossary (https://dart.dev/resources/glossary) but it doesn't contain either of these terms.
The diagnostic messages appear to be consistent in their use of 'unnamed' to mean a constructor that doesn't have a name, but are inconsistent with the use of 'default' to sometimes mean the synthetic constructor defined when there is no explicit constructor and sometimes to mean an unnamed zero parameter constructor.
I think we should be consistent with the spec, which uses 'default' for the synthetic constructor, 'unnamed' for a constructor that has no name, and doesn't have a term for a zero parameter constructor.
Brian WilkersonThanks! This all sounds reasonable to me. I thought we had previously said not to use unnamed. I searched my email to understand where that had come from and the only thing I found that was close to this was here:
https://github.com/dart-lang/sdk/issues/62393#issuecomment-3745407315
there's no such thing as an unnamed constructor
I think there might have been some discussions at the time around what the "name" was in a constructor (eg. in MyClass.foo()`), possibly because we'd been discussing navigation (or occurrences) for these things, so some of these things may have blurred together too.
Would it be useful to have these terms (and primary + secondary?) added to https://dart.dev/resources/glossary since if we use them in diagnostics/fixes, they're going to be used by users too?
... the only thing I found that was close to this was here ...
That is what the spec says (indirectly), but since then I've been leaning more toward using what I believe is the common meaning of the term and saying that an 'unnamed' constructor is a constructor whose name is the same as the class in which it's defined without a qualifying period and name.
Would it be useful to have these terms ...
I don't know. I think we only add entries for terms that might not be well-understood by our users, where "well-understood" is hard to quantify. It sounds like it would be a good question for the documentation team.
I feel like some of these might fall into that category. Seems there was an existing issue soliciting things to add to the glossary, so I've added them there for consideration. Thanks!
https://github.com/dart-lang/site-www/issues/6461#issuecomment-4156062692
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |