Issue 10883074: Update scan, changed and discard to handle "packages" and "pubspec.yaml"

Issue 10883074: Update scan, changed and discard to handle "packages" and "pubspec.yaml" (Closed)

Created:
8 years, 3 months ago by danrubel

Modified:
8 years, 3 months ago

Reviewers:
devoncarew, keertip

CC:
reviews_dartlang.org

Base URL:
http://dart.googlecode.com/svn/branches/bleeding_edge/dart/

Visibility:
Public.

More Reviews

Description

Update scan, changed and discard to handle "packages" directory Committed: https://code.google.com/p/dart/source/detail?r=12040

Patch Set 3 : #

Created: 8 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+410 lines, -283 lines)			Patch
M	editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/analysis/AnalysisServer.java	View	1	6 chunks	+51 lines, -43 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/analysis/Context.java	View	1	3 chunks	+82 lines, -7 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/analysis/DiscardTask.java	View	1 2	3 chunks	+10 lines, -90 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/analysis/EverythingChangedTask.java	View	1 2	2 chunks	+2 lines, -4 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/analysis/FileChangedTask.java	View	1	2 chunks	+16 lines, -19 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/analysis/SavedContext.java	View	1 2	4 chunks	+58 lines, -8 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/analysis/ScanTask.java	View	1	8 chunks	+15 lines, -11 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/analysis/AnalysisServerTest.java	View	1	6 chunks	+6 lines, -14 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/analysis/AnalysisTestUtilities.java	View	1	1 chunk	+0 lines, -7 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/analysis/DiscardTaskTest.java	View	1	8 chunks	+43 lines, -5 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/analysis/FileChangedTaskTest.java	View	1 2	3 chunks	+127 lines, -57 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/analysis/Listener.java	View	1	1 chunk	+0 lines, -18 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

danrubel

* modify scan, changed, and discard to create/dispose of application contexts * make some testing ...

8 years, 3 months ago (2012-08-27 23:03:00 UTC) #1

devoncarew

lgtm https://chromiumcodereview.appspot.com/10883074/diff/1/editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/analysis/AnalysisUtility.java File editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/analysis/AnalysisUtility.java (right): https://chromiumcodereview.appspot.com/10883074/diff/1/editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/analysis/AnalysisUtility.java#newcode102 editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/analysis/AnalysisUtility.java:102: return name.equals(DartCore.PACKAGES_DIRECTORY_NAME) || name.equals(DartCore.PUBSPEC_FILE_NAME); Should we test if ...

8 years, 3 months ago (2012-08-28 16:45:53 UTC) #2

keertip

https://chromiumcodereview.appspot.com/10883074/diff/1/editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/analysis/AnalysisUtility.java File editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/analysis/AnalysisUtility.java (right): https://chromiumcodereview.appspot.com/10883074/diff/1/editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/analysis/AnalysisUtility.java#newcode102 editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/analysis/AnalysisUtility.java:102: return name.equals(DartCore.PACKAGES_DIRECTORY_NAME) || name.equals(DartCore.PUBSPEC_FILE_NAME); DartCore.isPackagesDirectory() ? https://chromiumcodereview.appspot.com/10883074/diff/1/editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/analysis/FileChangedTask.java File editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/analysis/FileChangedTask.java ...

8 years, 3 months ago (2012-08-28 16:59:35 UTC) #3

danrubel

8 years, 3 months ago (2012-09-07 13:05:05 UTC) #4

https://chromiumcodereview.appspot.com/10883074/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/analysis/AnalysisUtility.java
(right):

https://chromiumcodereview.appspot.com/10883074/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/analysis/AnalysisUtility.java:102:
return name.equals(DartCore.PACKAGES_DIRECTORY_NAME) ||
name.equals(DartCore.PUBSPEC_FILE_NAME);
On 2012/08/28 16:59:35, keertip wrote:
> DartCore.isPackagesDirectory() ?

Per discussion, I don't need to check for changes to the pubspec file, so this
method has been simplified and inlined. In addition, I switched to using
isPackagesDirectory where I can.

https://chromiumcodereview.appspot.com/10883074/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/analysis/FileChangedTask.java
(right):

https://chromiumcodereview.appspot.com/10883074/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/analysis/FileChangedTask.java:49:
// If this is the "packages" directory or "pubspec.yaml" then reanalyze the
application
On 2012/08/28 16:59:35, keertip wrote:
> Changes to packages directory should be good enough. Changes to pubspec does
not
> imply changes to packages dir, until pub is run. 

Good point. Reverted and updated the AnalysisServer#scan method to check if the
"packages" directory is being added.

https://chromiumcodereview.appspot.com/10883074/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/internal/builder/DartBuilder.java
(right):

https://chromiumcodereview.appspot.com/10883074/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/internal/builder/DartBuilder.java:158:
server.changed(file);
On 2012/08/28 16:45:53, devoncarew wrote:
> Why don't we want changes from this directory passed in like the other
changes?
> 
> Ah, looks like any change in here and we discard the analysis state and
> recompute?

Yes, that was the plan, but after talking this over with Keerti, we've decided
to pass through the changes normally and let the analysis server update whats
added/changed and discard whats removed similar to other libraries.
Change reverted.

https://chromiumcodereview.appspot.com/10883074/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/internal/builder/DartBuilder.java:169:
if (name.endsWith(Extensions.DOT_DART) ||
name.equals(DartCore.PUBSPEC_FILE_NAME)) {
On 2012/08/28 16:59:35, keertip wrote:
> We don't have to look for changes to the pubspec file. Instead we can key of
on
> changes to the packages directory. 

Good point. Change reverted.

Expand Messages | Collapse Messages