Suggest a potential valid name for the flutter project when using `flutter create` #130900

ValentinVignal · 2023-07-19T16:19:02Z

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

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

andrewkolos

Thanks for the PR! I left some comments, requesting one change.

andrewkolos · 2023-07-20T22:20:08Z

packages/flutter_tools/lib/src/commands/create_base.dart

+  // Replaces all the non-alphanumeric characters with '_' and join the
+  // consecutive '_'s.
+  return newName.replaceAll(RegExp(r'[^a-z0-9]+'), '_');


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.

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 _

andrewkolos · 2023-07-20T22:21:33Z

packages/flutter_tools/lib/src/commands/create_base.dart

+      '"$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.',


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 👍 .

I copied the instructions in doc: Add more description to the error

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.

That makes sense. I changed the order in refactor: Put suggested name at the end of the message

andrewkolos · 2023-07-20T22:38:02Z

packages/flutter_tools/lib/src/commands/create_base.dart

+
+    return <String>[
+      '"$projectName" is not a valid Dart package name.',
+      if (isValidPackageName(potentialValidName)) '\nTry "$potentialValidName" instead.',


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.

Sure that makes sense, I made the change in refactor: Returns null if the name is not valid and only replace hyphens

…st-a-potential-valid-name

andrewkolos

LGTM with nits

andrewkolos · 2023-07-21T04:06:54Z

packages/flutter_tools/lib/src/commands/create_base.dart

+      '"$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.',


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.

andrewkolos · 2023-07-21T04:39:49Z

packages/flutter_tools/lib/src/commands/create_base.dart

+  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('-', '_');


Nit: These comments seem redundant with the code itself

Suggested change

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('-', '_');

Right, I got them removed in docs: Remove redundant comment

andrewkolos · 2023-07-21T04:46:42Z

packages/flutter_tools/test/general.shard/create_config_test.dart

    expect(isValidPackageName('Foo_bar'), false);
  });

+  test('Suggest a valid Pub package name', () {


Suggested change

test('Suggest a valid Pub package name', () {

test('Suggests a valid Pub package name', () {

Oops, sorry about that, fixed in docs: Fix typo in test description

…st-a-potential-valid-name

andrewkolos · 2023-07-21T17:57:11Z

Still LGTM. I've also restarted some checks that appeared to fail due to infra issues.

christopherfujino

LGTM

…sing `flutter create` (flutter/flutter#130900)

flutter/flutter@9cfbf6b...e8b397c 2023-07-22 [email protected] Setup leak tracking regression for material. (flutter/flutter#130169) 2023-07-22 [email protected] Roll Flutter Engine from 6344b17a2e03 to 481684a6e276 (2 revisions) (flutter/flutter#131118) 2023-07-22 [email protected] Roll Flutter Engine from b47cf14fda0e to 6344b17a2e03 (1 revision) (flutter/flutter#131114) 2023-07-22 [email protected] Roll Flutter Engine from 840bcc3449ff to b47cf14fda0e (3 revisions) (flutter/flutter#131109) 2023-07-22 [email protected] Roll Flutter Engine from 2d8cff44261b to 840bcc3449ff (11 revisions) (flutter/flutter#131101) 2023-07-21 [email protected] Remove obsolete work around for shadow drawing (flutter/flutter#131066) 2023-07-21 [email protected] Manual roll Flutter Engine from acb5d0640b6c to 2d8cff44261b (3 revisions) (flutter/flutter#131092) 2023-07-21 [email protected] Upgrade to newer leak_tracker. (flutter/flutter#131085) 2023-07-21 [email protected] Manual roll Flutter Engine from f5c1650c7acc to acb5d0640b6c (10 revisions) (flutter/flutter#131070) 2023-07-21 [email protected] Suggest a potential valid name for the flutter project when using `flutter create` (flutter/flutter#130900) 2023-07-21 [email protected] [CI/FTL] Oriole to Panther, presubmit false (flutter/flutter#130912) 2023-07-21 [email protected] 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 [email protected],[email protected] 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].*

ValentinVignal added 2 commits July 20, 2023 00:17

feat: suggests a potential valid name

c374a23

tests: update the tests

391d0f3

github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jul 19, 2023

ValentinVignal added 3 commits July 20, 2023 16:50

linter: Remove extra spaces

7af59bb

bug: Replace unknown characters for all names

a628767

fix: Add missing full stop

176aea3

andrewkolos self-requested a review July 20, 2023 22:01

andrewkolos requested changes Jul 20, 2023

View reviewed changes

ValentinVignal added 4 commits July 21, 2023 10:09

refactor: Returns null if the name is not valid and only replace hyphens

a5de39d

doc: Add more description to the error

14b2011

test: Update the tests

00c28a1

Merge remote-tracking branch 'upstream/main' into flutter-tools/sugge…

19bf06f

…st-a-potential-valid-name

ValentinVignal requested a review from andrewkolos July 21, 2023 02:14

andrewkolos approved these changes Jul 21, 2023

View reviewed changes

andrewkolos requested a review from christopherfujino July 21, 2023 05:02

ValentinVignal added 6 commits July 21, 2023 15:22

docs: Remove redundant comment

79de8e9

docs: Fix typo in test description

57d0758

refactor: Put suggested name at the end of the message

0dbc971

test: Update the tests

bcfa717

Merge remote-tracking branch 'upstream/main' into flutter-tools/sugge…

583a270

…st-a-potential-valid-name

lint: Remove trailing space

e82a902

christopherfujino approved these changes Jul 21, 2023

View reviewed changes

christopherfujino added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 21, 2023

auto-submit bot merged commit be7c7e3 into flutter:master Jul 21, 2023

engine-flutter-autoroll mentioned this pull request Jul 22, 2023

Roll Flutter from 9cfbf6b9fd40 to e8b397caca6f (12 revisions) flutter/packages#4550

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 22, 2023

be7c7e3 Suggest a potential valid name for the flutter project when u…

0e1077d

…sing `flutter create` (flutter/flutter#130900)

	test('Suggest a valid Pub package name', () {
	test('Suggests a valid Pub package name', () {

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

Uh oh!

Conversation

ValentinVignal commented Jul 19, 2023

Pre-launch Checklist

Uh oh!

andrewkolos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewkolos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewkolos commented Jul 21, 2023

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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