Preserve pedantic include in analysis_options.yaml #11
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
| final YamlMap currentAnalysisOptions = | ||
| loadAnalysisOptions(renameDeprecatedFilename: true); | ||
| final String currentAnalysisOptionsString = loadAnalysisOptionsAsString(); | ||
| Future<String> updateAnalysisOption(YamlMap abideYaml, |
There was a problem hiding this comment.
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.
| if (currentInclude != null && currentInclude.isNotEmpty) { | ||
| sb.writeln('include: $currentInclude'); | ||
| } | ||
| sb.write(''' |
There was a problem hiding this comment.
Here's the meat of this PR. Store the existing include if it is there, and emit it back to the file if present.
# Conflicts: # lib/src/update.dart
lib/src/update.dart
Outdated
| currentAnalysisOptionsString: currentAnalysisOptionsString); | ||
| lintErrorCounts = await getLintErrorCounts(abideYaml: abideYaml); | ||
| } | ||
| return writeAnalyisOptionsFile( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
lib/src/update.dart
Outdated
| String finalOutput = sb.toString(); | ||
| if (writeToFile) { | ||
| new File(analysisOptionsFilename).writeAsStringSync(finalOutput); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That's true it does feel weird to have a write to file option in a function named writeAnalysisOptions.
|
@georgelesica-wf I changed some things up based on your suggestion. How's this look? |
|
+1 lgtm |
|
@Workiva/release-management-pp |
Overview
Fixes #7
If an analysis options YAML has an include key it should be preserved in the output yaml file.
Some internal methods were modified to ease testing the update functionality
Testing