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
Suggest a potential valid name for the flutter project when using flutter create
#130900
Conversation
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.
Thanks for the PR! I left some comments, requesting one change.
// Replaces all the non-alphanumeric characters with '_' and join the | ||
// consecutive '_'s. | ||
return newName.replaceAll(RegExp(r'[^a-z0-9]+'), '_'); |
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 in experiment_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 _
'"$projectName" is not a valid Dart package name.', | ||
if (isValidPackageName(potentialValidName)) '\nTry "$potentialValidName" instead.', | ||
'\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 comment
The 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 comment
The 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 comment
The 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 comment
The 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
|
||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick:
I am not sure I like that potentialValidPackageName
can return a name that's still invalid. That could be confusing given its name.
Rather than checking potentialValidName
with isValidPackageName
, how about having potentialValidPackageName
do the check instead? Then, if the new name that potentialValidPackageName
finds still isn't valid, it can instead return null
.
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.
Sure that makes sense, I made the change in refactor: Returns null if the name is not valid and only replace hyphens
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.
LGTM with nits
'"$projectName" is not a valid Dart package name.', | ||
if (isValidPackageName(potentialValidName)) '\nTry "$potentialValidName" instead.', | ||
'\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 comment
The 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.
if (newName.startsWith(RegExp(r'[0-9]'))) { | ||
// If the package starts with a number, prepend '_'. | ||
newName = '_$newName'; | ||
} | ||
// Replaces all hyphens ('-') characters with '_'. | ||
newName = newName.replaceAll('-', '_'); |
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.
Nit: These comments seem redundant with the code itself
if (newName.startsWith(RegExp(r'[0-9]'))) { | |
// If the package starts with a number, prepend '_'. | |
newName = '_$newName'; | |
} | |
// Replaces all hyphens ('-') characters with '_'. | |
newName = newName.replaceAll('-', '_'); | |
if (newName.startsWith(RegExp(r'[0-9]'))) { | |
newName = '_$newName'; | |
} | |
newName = newName.replaceAll('-', '_'); |
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.
Right, I got them removed in docs: Remove redundant comment
@@ -20,6 +20,18 @@ 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 comment
The reason will be displayed to describe this comment to others. Learn more.
test('Suggest a valid Pub package name', () { | |
test('Suggests a valid Pub package name', () { |
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.
Oops, sorry about that, fixed in docs: Fix typo in test description
…st-a-potential-valid-name
Still LGTM. I've also restarted some checks that appeared to fail due to infra issues. |
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.
LGTM
… using `flutter create` (flutter/flutter#130900)
flutter/flutter@9cfbf6b...e8b397c 2023-07-22 polinach@google.com Setup leak tracking regression for material. (flutter/flutter#130169) 2023-07-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6344b17a2e03 to 481684a6e276 (2 revisions) (flutter/flutter#131118) 2023-07-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from b47cf14fda0e to 6344b17a2e03 (1 revision) (flutter/flutter#131114) 2023-07-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 840bcc3449ff to b47cf14fda0e (3 revisions) (flutter/flutter#131109) 2023-07-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2d8cff44261b to 840bcc3449ff (11 revisions) (flutter/flutter#131101) 2023-07-21 goderbauer@google.com Remove obsolete work around for shadow drawing (flutter/flutter#131066) 2023-07-21 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from acb5d0640b6c to 2d8cff44261b (3 revisions) (flutter/flutter#131092) 2023-07-21 polinach@google.com Upgrade to newer leak_tracker. (flutter/flutter#131085) 2023-07-21 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from f5c1650c7acc to acb5d0640b6c (10 revisions) (flutter/flutter#131070) 2023-07-21 32538273+ValentinVignal@users.noreply.github.com Suggest a potential valid name for the flutter project when using `flutter create` (flutter/flutter#130900) 2023-07-21 dnfield@google.com [CI/FTL] Oriole to Panther, presubmit false (flutter/flutter#130912) 2023-07-21 6655696+guidezpl@users.noreply.github.com Improve handling of certain icons in RTL (flutter/flutter#130979) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…utter create` (flutter#130900) Fixes flutter#109775 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*
…utter create` (flutter#130900) Fixes flutter#109775 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*
Fixes #109775
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.