Analyzer skeleton. #149
Analyzer skeleton. #149
Conversation
app/lib/analyzer.dart
Outdated
app/lib/analyzer.dart
Outdated
app/lib/analyzer.dart
Outdated
There was a problem hiding this comment.
I have all of this written already - https://github.com/kevmoo/pana.dart
Mostly. Need to expose some of the private impl stuff I have
There was a problem hiding this comment.
looks good! Any ETA for publishing the recent changes?
There was a problem hiding this comment.
@kevmoo No you did not! The last upload to package:pana was in 2015!
There was a problem hiding this comment.
Also if we want to use package:pana for the task, why this code here?
There was a problem hiding this comment.
This code precedes my awareness of pana. I think pana is a better place for most of this analysis, and I'm planning to contribute to it (e.g. dart-lang/pana#3). At the same time as soon as it is 'good enough', I'll update this PR with the 'pana' version.
|
@mkustermann would you take a look and merge? |
| } | ||
|
|
||
| class PubspecLock { | ||
| Map _content; |
There was a problem hiding this comment.
Since you initialize it in the constructor, you might as well make it final (and use initializer rather than constructor body)
| 'with exit code: ${result.exitCode}\n' | ||
| 'stdout: ${result.stdout}\n' | ||
| 'stderr: ${result.stderr}'); | ||
| throw new Exception('Failed to extract tarball.'); |
There was a problem hiding this comment.
Consider changing readTarballFile to also log a warning - for consistency.
| } | ||
|
|
||
| List<LockedDependency> get packages { | ||
| if (_packages == null) { |
There was a problem hiding this comment.
Why do this lazily? The only reason one would construct a PubspecLock object is to call packages on it.
| // for details. All rights reserved. Use of this source code is governed by a | ||
| // BSD-style license that can be found in the LICENSE file. | ||
|
|
||
| library pub_dartlang_org.analysis; |
There was a problem hiding this comment.
Maybe use a consistent name for the filename and library name (i.e. analysis -> analyzer).
| ? new Directory(workDir) | ||
| : Directory.systemTemp.createTempSync(); | ||
| return new Analyzer._(new File(source), wd, workDir == null); | ||
| } |
There was a problem hiding this comment.
Maybe it would be easier to read when using only one instead of two constructors:
Analyzer(String source, {String workdir})
: _sourceTarGz = new File(source),
_workDir = workdir != null ? new Directory(workdir) : Directory.systemTemp.createTempSync(),
_clearWorkDir = workdir == null;| } | ||
|
|
||
| /// Prepares the directories for the analysis and extracts the package. | ||
| Future setUp() async { |
There was a problem hiding this comment.
Consider renaming it from setUp to extractTarball.
app/lib/analyzer.dart
Outdated
There was a problem hiding this comment.
@kevmoo No you did not! The last upload to package:pana was in 2015!
app/lib/analyzer.dart
Outdated
There was a problem hiding this comment.
Also if we want to use package:pana for the task, why this code here?
| /// Reads pubspec.yaml and pubspec.lock. | ||
| Future readPubspecs() async { | ||
| final String pubspecContent = await new File( | ||
| _packageDir.path + Platform.pathSeparator + 'pubspec.yaml') |
There was a problem hiding this comment.
Consider using package:path/path.dart and do path.join(_packagesDir.path, 'pubspec.yaml')
|
I believe that the recent changes and the two open PRs at https://github.com/kevmoo/pana.dart cover more than I was planning here. As soon as there is a new version, I'll close this PR and create a new one using |
|
|
4 participants
I wanted it to structure it in a way that helps to develop its parts in parallel.