-
Notifications
You must be signed in to change notification settings - Fork 26.7k
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
Suggest a potential valid name for the flutter project when using flutter create
#130900
Changes from 5 commits
c374a23
391d0f3
7af59bb
a628767
176aea3
a5de39d
14b2011
00c28a1
19bf06f
79de8e9
57d0758
0dbc971
bcfa717
583a270
e82a902
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -784,12 +784,31 @@ bool isValidPackageName(String name) { | |
!_keywords.contains(name); | ||
} | ||
|
||
/// Returns a potential valid name from the given [name]. | ||
@visibleForTesting | ||
String potentialValidPackageName(String name){ | ||
String newName = name.toLowerCase(); | ||
if (newName.startsWith(RegExp(r'[0-9]'))) { | ||
// If the package starts with a number, prepend '_'. | ||
newName = '_$newName'; | ||
} | ||
// Replaces all the non-alphanumeric characters with '_' and join the | ||
// consecutive '_'s. | ||
return newName.replaceAll(RegExp(r'[^a-z0-9]+'), '_'); | ||
} | ||
|
||
// Return null if the project name is legal. Return a validation message if | ||
// we should disallow the project name. | ||
String? _validateProjectName(String projectName) { | ||
if (!isValidPackageName(projectName)) { | ||
return '"$projectName" is not a valid Dart package name.\n\n' | ||
'See https://dart.dev/tools/pub/pubspec#name for more information.'; | ||
final String potentialValidName = potentialValidPackageName(projectName); | ||
|
||
return <String>[ | ||
'"$projectName" is not a valid Dart package name.', | ||
if (isValidPackageName(potentialValidName)) '\nTry "$potentialValidName" instead.', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: I am not sure I like that Rather than checking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure that makes sense, I made the change in refactor: Returns null if the name is not valid and only replace hyphens |
||
'\n\n', | ||
'See https://dart.dev/tools/pub/pubspec#name for more information.', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could also put the rules directly into the output so the user have to leave their terminal. I doubt these rules would ever change, so we don't have to worry about them becoming out of date. However, I think just having a link is also sufficient 👍 . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I copied the instructions in doc: Add more description to the error There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: If we do include the rules and the link, I think we should include the suggested name last. The suggested name is the least important information. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense. I changed the order in refactor: Put suggested name at the end of the message |
||
].join(); | ||
} | ||
if (_packageDependencies.contains(projectName)) { | ||
return "Invalid project name: '$projectName' - this will conflict with Flutter " | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -20,6 +20,17 @@ void main() { | |||||
expect(isValidPackageName('Foo_bar'), false); | ||||||
}); | ||||||
|
||||||
test('Suggest a valid Pub package name', () { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, sorry about that, fixed in docs: Fix typo in test description |
||||||
expect(potentialValidPackageName('92'), '_92'); | ||||||
expect(potentialValidPackageName('a-b-c'), 'a_b_c'); | ||||||
|
||||||
|
||||||
expect(potentialValidPackageName('foo.bar'), 'foo_bar'); | ||||||
expect(potentialValidPackageName('Foo_bar'), 'foo_bar'); | ||||||
expect(potentialValidPackageName('foo._bar'), 'foo_bar'); | ||||||
|
||||||
}); | ||||||
|
||||||
test('kWindowsDrivePattern', () { | ||||||
expect(CreateBase.kWindowsDrivePattern.hasMatch(r'D:\'), isFalse); | ||||||
expect(CreateBase.kWindowsDrivePattern.hasMatch(r'z:\'), isFalse); | ||||||
|
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.
I think replacing hyphens (
-
) with underscores (_
) is definitely good. But I am not sure about all nonalphanumeric characters.For example,
experiment@2023-7-20@v3
would result inexperiment_2023_7_20_v3
, which I think is a decent suggestion.However,
잘못된 이름
would result in______
, which is not a useful suggestion. I think this kind of case is probably more rare, but is still worth considering.I think we should only try to replace hyphens, to keep this from being confusing or controversial.
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.
Fair enough, I've made the change in refactor: Returns null if the name is not valid and only replace hyphens to simplify the logic and only replace
-
with_