Skip to content
This repository has been archived by the owner on Jul 2, 2024. It is now read-only.

Preserve pedantic include in analysis_options.yaml #11

Merged
merged 7 commits into from
Feb 27, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
FROM google/dart:2.0.0 as build
FROM google/dart:2.1.1 as build

# Build Environment Vars
ARG BUILD_ID
Expand Down
56 changes: 38 additions & 18 deletions lib/src/update.dart
Original file line number Diff line number Diff line change
Expand Up @@ -19,48 +19,65 @@ import 'package:yaml/yaml.dart';
import 'package:abide/src/constants.dart';
import 'package:abide/src/util.dart';

Future<Null> updateAnalysisOption(YamlMap abideYaml,
{bool uncommentClean}) async {
final YamlMap currentAnalysisOptions =
loadAnalysisOptions(renameDeprecatedFilename: true);
final String currentAnalysisOptionsString = loadAnalysisOptionsAsString();
Future<String> updateAnalysisOption(YamlMap abideYaml,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the return type to return the analysis options file contents as a String. This aids in testing since you don't actually have to do filesystem operations. Before it would assume it would read and write the file to the current directory. While that was convenient, it made running tests difficult. Changing the working directory changes it for the whole process. That meant tests couldn't be run concurrently. Instead, I changed the functions to return the content and have a flag on whether or not to actually write to the filesystem and the path to the file to read from.

{String pathToAnalysisOptionsFile,
bool uncommentClean,
bool writeToFile = true}) async {
YamlMap currentAnalysisOptions = loadAnalysisOptions(
pathToAnalysisOptionsFile: pathToAnalysisOptionsFile,
renameDeprecatedFilename: true);

final String currentAnalysisOptionsString = loadAnalysisOptionsAsString(
pathToAnalysisOptionsFile: pathToAnalysisOptionsFile);

Map<String, Map<String, int>> lintErrorCounts = <String, Map<String, int>>{};
// Write a version with ALL lint rules enabled
// and then run dart analyzer to get counts of lint per rule
// those counts are passed into the 2nd write run to decide
// whether or not to comment out a lint rule with a lot of
// new lint warnings
writeAnalyisOptionsFile(
all: true,
abideYaml: abideYaml,
currentAnalysisOptions: currentAnalysisOptions,
currentAnalysisOptionsString: currentAnalysisOptionsString);
lintErrorCounts = await getLintErrorCounts(abideYaml: abideYaml);
writeAnalyisOptionsFile(
// new lint warnings. If we're not writing to a file, then lint
// counts won't work, so skip that step.
if (writeToFile) {
writeAnalyisOptionsFile(
all: true,
abideYaml: abideYaml,
currentAnalysisOptions: currentAnalysisOptions,
currentAnalysisOptionsString: currentAnalysisOptionsString);
lintErrorCounts = await getLintErrorCounts(abideYaml: abideYaml);
}
return writeAnalyisOptionsFile(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kinda weird that it runs this function twice. Would it make more sense to return an object that reflects the changes to be made and then have a separate function that writes them to disk? That would also keep the filesystem code paths separate from the more testable non-filesystem code paths.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It always ran twice before. The getLintErrorCounts runs the command line dart analyzer to get the number of lints for each rule. That tool needs the analysis options file written to disk. So the first run writes a file with ALL the rules enabled to collect counts for all the rules. Then the 2nd writes it with some commented out due to the recommendations or existing lints hit.
The change here was to not do the first run while testing because we're not writing to disk. (The writeToFile option is only for testing)

abideYaml: abideYaml,
currentAnalysisOptions: currentAnalysisOptions,
currentAnalysisOptionsString: currentAnalysisOptionsString,
lintErrorCounts: lintErrorCounts,
uncommentClean: uncommentClean);
uncommentClean: uncommentClean,
writeToFile: writeToFile);
}

void writeAnalyisOptionsFile(
String writeAnalyisOptionsFile(
{YamlMap abideYaml,
YamlMap currentAnalysisOptions,
String currentAnalysisOptionsString,
bool all = false,
bool uncommentClean = false,
Map<String, Map<String, int>> lintErrorCounts:
bool writeToFile = true,
Map<String, Map<String, int>> lintErrorCounts =
const <String, Map<String, int>>{}}) {
currentAnalysisOptions ??= new YamlMap();

final String currentInclude = getYamlValue(currentAnalysisOptions, 'include');
final bool currentImplicitCasts = getYamlValue(
currentAnalysisOptions, 'analyzer:strong-mode:implicit-casts', true);
final bool currentImplicitDynamic = getYamlValue(
currentAnalysisOptions, 'analyzer:strong-mode:implicit-dynamic', true);

final StringBuffer sb = new StringBuffer('''
# analysis_options.yaml docs: https://www.dartlang.org/guides/language/analysis-options
''');
if (currentInclude != null && currentInclude.isNotEmpty) {
sb.writeln('include: $currentInclude');
}
sb.write('''
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the meat of this PR. Store the existing include if it is there, and emit it back to the file if present.

analyzer:
# Strong mode is required. Applies to the current project.
strong-mode:
Expand Down Expand Up @@ -198,15 +215,18 @@ linter:
$output
''');
}

new File(analysisOptionsFilename).writeAsStringSync(sb.toString());
String finalOutput = sb.toString();
if (writeToFile) {
new File(analysisOptionsFilename).writeAsStringSync(finalOutput);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like just returning the value instead of optionally writing it might make the code cleaner. The actual file operation could be totally separate then, and it could even be tested on its own pretty easily.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true it does feel weird to have a write to file option in a function named writeAnalysisOptions.

if (!all) {
print('Wrote $analysisOptionsFilename');
}
if (nMissingRecommendations > 0) {
print(
'There were missing recommendations. Please inform the maintainers of the abide tool to perform an abide upgrade.');
}
return finalOutput;
}

String _lintResultFor(String lint, Map<String, Map<String, int>> lintErrors) {
Expand Down
15 changes: 11 additions & 4 deletions lib/src/util.dart
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,19 @@ YamlMap loadPubspec() => loadYamlFile(pubspecFilename);
YamlMap loadSmithy() =>
loadYamlFile(smithyFilename) ?? loadYamlFile(smithyFilename2);

YamlMap loadAnalysisOptions({bool renameDeprecatedFilename: false}) =>
loadYamlFile(findAnalysisOptionsFile(
YamlMap loadAnalysisOptions(
{String pathToAnalysisOptionsFile, bool renameDeprecatedFilename: false}) {
if (pathToAnalysisOptionsFile != null) {
return loadYamlFile(pathToAnalysisOptionsFile);
} else {
return loadYamlFile(findAnalysisOptionsFile(
renameDeprecatedFilename: renameDeprecatedFilename));
}
}

String loadAnalysisOptionsAsString() {
final String filename = findAnalysisOptionsFile();
String loadAnalysisOptionsAsString({String pathToAnalysisOptionsFile}) {
final String filename =
pathToAnalysisOptionsFile ?? findAnalysisOptionsFile();
if (filename == null) {
return '';
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
include: package:pedantic/analysis_options.yaml
2 changes: 1 addition & 1 deletion test/vm/check_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ Future<Null> main() async {
}
});

group('Verifiy pubspec check', () {
group('Verify pubspec check', () {
final YamlMap pubspec = loadPubspec();
final YamlMap pubspecWithMissingDeps =
loadYamlFile('test/fixtures/pubspec_missing_deps.yaml');
Expand Down
38 changes: 38 additions & 0 deletions test/vm/update_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Copyright 2017-2019 Workiva Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

@TestOn('vm')
import 'dart:async';
import 'package:test/test.dart';
import 'package:yaml/yaml.dart';

import 'package:abide/src/util.dart';
import 'package:abide/src/update.dart';

Future<Null> main() async {
YamlMap abideYaml = await loadAbideYaml();

group('Update', () {
test('preserves an existing include key', () async {
String result = await updateAnalysisOption(abideYaml,
pathToAnalysisOptionsFile:
'test/fixtures/analysis/includes_pedantic/analysis_options.yaml',
uncommentClean: false,
writeToFile: false);
YamlMap yamlOutput = loadYaml(result);
expect(yamlOutput['include'] == 'package:pedantic/analysis_options.yaml',
isTrue);
});
});
}