Handle file additions/removals in ContextDirectoryManager.

Handle file additions/removals in ContextDirectoryManager.

R=danrubel@google.com, scheglov@google.com

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

[scheglov, 2014-05-29 17:37:03]

https://codereview.chromium.org/308713002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/context_directory_manager.dart (right):

https://codereview.chromium.org/308713002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/context_directory_manager.dart:104: String shortName
= resourceProvider.pathContext.basename(event.path);
Should we hide using "path" package and just add such function into
ResourceProvider?

Actually there is Resource.shortName already.

https://codereview.chromium.org/308713002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/context_directory_manager.dart:110: if (resource is
File) {
AFAIK getResource() will always return a File, it is up to the client to check
if the resource exists or not.

https://codereview.chromium.org/308713002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/context_directory_manager.dart:148: //
child.fullName to child.path.
SGTM

https://codereview.chromium.org/308713002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/context_directory_manager.dart:157: static bool
_shouldFileBeAnalyzed(String shortName) {
I don't think isDartFileName() cares if it is given a short name or a full name.
So, might not need to get a basename() at all.

[Paul Berry, 2014-05-29 17:57:21]

https://codereview.chromium.org/308713002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/context_directory_manager.dart (right):

https://codereview.chromium.org/308713002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/context_directory_manager.dart:104: String shortName
= resourceProvider.pathContext.basename(event.path);
On 2014/05/29 17:37:03, scheglov wrote:
> Should we hide using "path" package and just add such function into
> ResourceProvider?

That's not a bad idea--it would decrease our risk of mistakenly using "path" and
leading to subtle bugs.

> 
> Actually there is Resource.shortName already.

Yes but at this point in the code we haven't created the Resource yet.  And I
don't think we should create the Resource until we've decided that it should be
analyzed, because creating the Resource requires hitting the filesystem, and I
don't want to introduce unnecessary I/O on files that we aren't interested in
analyzing.

I guess this is a moot point considering your comment below that we don't need
to use basename().

https://codereview.chromium.org/308713002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/context_directory_manager.dart:110: if (resource is
File) {
On 2014/05/29 17:37:03, scheglov wrote:
> AFAIK getResource() will always return a File, it is up to the client to check
> if the resource exists or not.

Ok, my comment is wrong, but I believe this if-test is still needed, because
getResource() might also return a Folder.

https://codereview.chromium.org/308713002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/context_directory_manager.dart:157: static bool
_shouldFileBeAnalyzed(String shortName) {
On 2014/05/29 17:37:03, scheglov wrote:
> I don't think isDartFileName() cares if it is given a short name or a full
name.
> So, might not need to get a basename() at all.

You're right.  I'll rework this to remove the call to basename().

[Paul Berry, 2014-05-29 18:09:11]

PTAL

[danrubel, 2014-05-29 18:14:43]

lgtm

https://codereview.chromium.org/308713002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/context_directory_manager.dart (right):

https://codereview.chromium.org/308713002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/context_directory_manager.dart:41: <Folder,
_ContextDirectoryInfo>{};
Do you need the extra type declaration here since the map is empty?
I find this easier to read...

Map<Folder, _ContextDirectoryInfo> _currentDirectoryInfo = {};

[scheglov, 2014-05-29 18:19:03]

LGTM

https://codereview.chromium.org/308713002/diff/20001/pkg/analysis_server/lib/...
File pkg/analysis_server/lib/src/context_directory_manager.dart (right):

https://codereview.chromium.org/308713002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/context_directory_manager.dart:153: static bool
_shouldFileBeAnalyzed(String shortName) {
shortName -> path

https://codereview.chromium.org/308713002/diff/20001/pkg/analysis_server/lib/...
File pkg/analysis_server/lib/src/resource.dart (right):

https://codereview.chromium.org/308713002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/resource.dart:104: String get path => _path;
We could just drop this getter now and rename _path to "path".

[Paul Berry, 2014-05-29 18:30:22]

https://codereview.chromium.org/308713002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/context_directory_manager.dart (right):

https://codereview.chromium.org/308713002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/context_directory_manager.dart:41: <Folder,
_ContextDirectoryInfo>{};
On 2014/05/29 18:14:43, danrubel wrote:
> Do you need the extra type declaration here since the map is empty?
> I find this easier to read...
> 
> Map<Folder, _ContextDirectoryInfo> _currentDirectoryInfo = {};

Heh, I was just talking about this yesterday with Konstantin, Jaime, and Brian
:)

Unfortunately, both type annotations are needed.  The one on the left prevents
later code from assigning a non-map (or a map of the wrong type) to
_currentDirectoryInfo.  The one on the right affects the runtime type of the
map, which means that in checked mode we get additional runtime type checks
whenever the map is modified (e.g. "_currentDirectoryInfo[a] = b" will check the
types of a and b).  Since the type system is unsound, the additional runtime
type checks can actually make a difference.

[danrubel, 2014-05-29 18:55:46]

On 2014/05/29 18:30:22, Paul Berry wrote:
>
https://codereview.chromium.org/308713002/diff/1/pkg/analysis_server/lib/src/...
> File pkg/analysis_server/lib/src/context_directory_manager.dart (right):
> 
>
https://codereview.chromium.org/308713002/diff/1/pkg/analysis_server/lib/src/...
> pkg/analysis_server/lib/src/context_directory_manager.dart:41: <Folder,
> _ContextDirectoryInfo>{};
> On 2014/05/29 18:14:43, danrubel wrote:
> > Do you need the extra type declaration here since the map is empty?
> > I find this easier to read...
> > 
> > Map<Folder, _ContextDirectoryInfo> _currentDirectoryInfo = {};
> 
> Heh, I was just talking about this yesterday with Konstantin, Jaime, and Brian
> :)
> 
> Unfortunately, both type annotations are needed.  The one on the left prevents
> later code from assigning a non-map (or a map of the wrong type) to
> _currentDirectoryInfo.  The one on the right affects the runtime type of the
> map, which means that in checked mode we get additional runtime type checks
> whenever the map is modified (e.g. "_currentDirectoryInfo[a] = b" will check
the
> types of a and b).  Since the type system is unsound, the additional runtime
> type checks can actually make a difference.

Sgtm.

[Paul Berry, 2014-05-29 20:14:29]

Committed patchset #2 manually as r36800 (presubmit successful).