Issue 15037. Warn users if they have “assets” dir anywhere inside “web” dir (http://dartbug.com/150…

Issue 15037. Warn users if they have “assets” dir anywhere inside “web” dir (http://dartbug.com/15037).

BUG=
R=rnystrom@google.com

Committed: https://code.google.com/p/dart/source/detail?r=31292

[Bob Nystrom, 2013-12-16 18:02:14]

This is excellent! A couple of minor tweaks and then I will be delighted to land
this.

Thank you!

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:253: void _checkAssetsDir() {
How about a more specific name here. Maybe "_warnOnAssetsPaths()"?

At the very least, we shouldn't use "dir" since this also warns on files named
"assets".

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:261: 'Warning: directory $p
confuses pub, please use different name for '
Let's quote the path name. The message is a bit ambiguous too. How about:

'Warning: Pub reserves paths containing "assets" for using assets from packages.
Please rename the path "$p".'

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:267: /// dependencies. Before
loading makes sure the lockfile is up to date.
"lockfile is up to date" -> "lockfile and dependencies are installed and up to
date".

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:282: var packageMap = new
Map.fromIterable(packages,
Nice. :)

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:284: value: (p) => p);
You can leave off the "value:" parameter. It will default to that if omitted.

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/test/b...
File
sdk/lib/_internal/pub/test/build/warns_about_assets_dir_while_build_test.dart
(right):

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/test/b...
sdk/lib/_internal/pub/test/build/warns_about_assets_dir_while_build_test.dart:3:
// BSD-style license that can be found in the LICENSE file.
Since this is in the "build" directory, "_while_build" is unneeded in the test
name.

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/test/b...
sdk/lib/_internal/pub/test/build/warns_about_assets_dir_while_build_test.dart:13:
integration("warns user about assets dir in the root of the web", () {
the web -> 'web'

Here and below.

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/test/b...
sdk/lib/_internal/pub/test/build/warns_about_assets_dir_while_build_test.dart:23:
error: new RegExp(WARNING_PATTERN),
It would be good to validate that the warning message includes the correct path
too.

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/test/b...
sdk/lib/_internal/pub/test/build/warns_about_assets_dir_while_build_test.dart:73:
integration("keeps silence if no assets dir or file anywhere in the web", () {
"keeps silence" -> "does not warn".

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/test/s...
File
sdk/lib/_internal/pub/test/serve/warns_about_assets_dir_while_serve_test.dart
(right):

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/warns_about_assets_dir_while_serve_test.dart:3:
// BSD-style license that can be found in the LICENSE file.
And, of course, my comments on the previous file apply here too. :)

[nweiz, 2013-12-16 22:39:21]

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:270: .then((_) =>
_checkAssetsDir())
Since this isn't actually asynchronous, it can be in the same [then] callback as
[_loadPackageGraph].

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/test/b...
File
sdk/lib/_internal/pub/test/build/warns_about_assets_dir_while_build_test.dart
(right):

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/test/b...
sdk/lib/_internal/pub/test/build/warns_about_assets_dir_while_build_test.dart:8:
const WARNING_PATTERN = r'Warning: directory (.*) confuses pub';
Make this a RegExp. It's fine if it's not const, although you'll also want to
change the name.

Per Bob's comment about the path below, maybe the thing to do is just to make
this a function that takes the path. and returns a regexp.

[Anton Moiseev, 2013-12-17 13:53:00]

Thank you for your detailed feedback. Addressed all the comments.

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:253: void _checkAssetsDir() {
On 2013/12/16 18:02:14, Bob Nystrom wrote:
> How about a more specific name here. Maybe "_warnOnAssetsPaths()"?
> 
> At the very least, we shouldn't use "dir" since this also warns on files named
> "assets".

Right, thanks! Named it before realized we should warn on files as well.

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:261: 'Warning: directory $p
confuses pub, please use different name for '
On 2013/12/16 18:02:14, Bob Nystrom wrote:
> Let's quote the path name. The message is a bit ambiguous too. How about:
> 
> 'Warning: Pub reserves paths containing "assets" for using assets from
packages.
> Please rename the path "$p".'

Explaining why pub gets confused is definitely more clear for user. Thanks!

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:267: /// dependencies. Before
loading makes sure the lockfile is up to date.
On 2013/12/16 18:02:14, Bob Nystrom wrote:
> "lockfile is up to date" -> "lockfile and dependencies are installed and up to
> date".

Done.

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:270: .then((_) =>
_checkAssetsDir())
On 2013/12/16 22:39:22, nweiz wrote:
> Since this isn't actually asynchronous, it can be in the same [then] callback
as
> [_loadPackageGraph].

Done.

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:284: value: (p) => p);
On 2013/12/16 18:02:14, Bob Nystrom wrote:
> You can leave off the "value:" parameter. It will default to that if omitted.

Even more concise. Love one-liners, especially when they remain readable :)

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/test/b...
File
sdk/lib/_internal/pub/test/build/warns_about_assets_dir_while_build_test.dart
(right):

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/test/b...
sdk/lib/_internal/pub/test/build/warns_about_assets_dir_while_build_test.dart:3:
// BSD-style license that can be found in the LICENSE file.
On 2013/12/16 18:02:14, Bob Nystrom wrote:
> Since this is in the "build" directory, "_while_build" is unneeded in the test
> name.

Agree. Renamed to match method name - warns_on_assets_paths_test.dart

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/test/b...
sdk/lib/_internal/pub/test/build/warns_about_assets_dir_while_build_test.dart:8:
const WARNING_PATTERN = r'Warning: directory (.*) confuses pub';
On 2013/12/16 22:39:22, nweiz wrote:
> Make this a RegExp. It's fine if it's not const, although you'll also want to
> change the name.
> 
> Per Bob's comment about the path below, maybe the thing to do is just to make
> this a function that takes the path. and returns a regexp.

Right, a function - is what I was thinking of as well when Bob suggested to
validate path.

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/test/b...
sdk/lib/_internal/pub/test/build/warns_about_assets_dir_while_build_test.dart:13:
integration("warns user about assets dir in the root of the web", () {
On 2013/12/16 18:02:14, Bob Nystrom wrote:
> the web -> 'web'
> 
> Here and below.

Done. Also unified usage of ' and " within the file. BTW, do you have a
preference what quotes (single or double) to use for pub's source code? Haven't
found it in the style guide.

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/test/b...
sdk/lib/_internal/pub/test/build/warns_about_assets_dir_while_build_test.dart:23:
error: new RegExp(WARNING_PATTERN),
On 2013/12/16 18:02:14, Bob Nystrom wrote:
> It would be good to validate that the warning message includes the correct
path
> too.

Good point, haven't thought about it. Thanks!

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/test/b...
sdk/lib/_internal/pub/test/build/warns_about_assets_dir_while_build_test.dart:73:
integration("keeps silence if no assets dir or file anywhere in the web", () {
On 2013/12/16 18:02:14, Bob Nystrom wrote:
> "keeps silence" -> "does not warn".

Done.

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/test/s...
File
sdk/lib/_internal/pub/test/serve/warns_about_assets_dir_while_serve_test.dart
(right):

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/warns_about_assets_dir_while_serve_test.dart:3:
// BSD-style license that can be found in the LICENSE file.
On 2013/12/16 18:02:14, Bob Nystrom wrote:
> And, of course, my comments on the previous file apply here too. :)

Done.

[Bob Nystrom, 2013-12-18 23:45:48]

One more request, then I think this will be ready to land. :)

- bob

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/test/b...
File
sdk/lib/_internal/pub/test/build/warns_about_assets_dir_while_build_test.dart
(right):

https://codereview.chromium.org/110013003/diff/1/sdk/lib/_internal/pub/test/b...
sdk/lib/_internal/pub/test/build/warns_about_assets_dir_while_build_test.dart:13:
integration("warns user about assets dir in the root of the web", () {
On 2013/12/17 13:53:01, Anton Moiseev wrote:
> On 2013/12/16 18:02:14, Bob Nystrom wrote:
> > the web -> 'web'
> > 
> > Here and below.
> 
> Done. Also unified usage of ' and " within the file. BTW, do you have a
> preference what quotes (single or double) to use for pub's source code?
Haven't
> found it in the style guide.

We don't, actually. We've talked about it a bunch, but we've never figure out
what the preference should be. Generally, we try to avoid having to escape
quotes, so if the string contains a ', we'll use "" for the string itself and
vice versa. Outside of those cases, though, we don't have any rules yet.

https://codereview.chromium.org/110013003/diff/10001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/build/warns_on_assets_paths_test.dart (right):

https://codereview.chromium.org/110013003/diff/10001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/build/warns_on_assets_paths_test.dart:17: // need to
accomodate that.
Does this imply that the path in the error message will be absolute? I think it
would be friendlier to users if it was relative to the package root directory.

That should also let you get rid of this too.

[Anton Moiseev, 2013-12-19 10:09:08]

https://codereview.chromium.org/110013003/diff/10001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/build/warns_on_assets_paths_test.dart (right):

https://codereview.chromium.org/110013003/diff/10001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/build/warns_on_assets_paths_test.dart:17: // need to
accomodate that.
On 2013/12/18 23:45:48, Bob Nystrom wrote:
> Does this imply that the path in the error message will be absolute? I think
it
> would be friendlier to users if it was relative to the package root directory.
> 
> That should also let you get rid of this too.

Agree. Thanks for pointing that out. Fixed.

[Bob Nystrom, 2013-12-19 18:18:48]

Awesome, LGTM! I'll be delighted to land this.

Thank you!

[Bob Nystrom, 2013-12-19 18:43:12]

Committed patchset #4 manually as r31292 (presubmit successful).