Skip to content

Get rid of a backlog of potentially suboptimal coding practices in dartdoc #2830

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 8, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
58 changes: 16 additions & 42 deletions analysis_options.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Change analysis_options.yaml and analysis_options_presubmit.yaml
# together.
include: package:pedantic/analysis_options.1.11.0.yaml
include: package:lints/recommended.yaml

analyzer:
errors:
Expand All @@ -19,45 +19,19 @@ analyzer:
- 'testing/test_package_export_error/**'
linter:
rules:
- always_declare_return_types
- avoid_dynamic_calls
- avoid_single_cascade_in_expression_statements
- avoid_unused_constructor_parameters
- annotate_overrides
- avoid_init_to_null
- directives_ordering
- no_adjacent_strings_in_list
- package_api_docs
- prefer_final_fields
- prefer_initializing_formals
- prefer_void_to_null
- slash_for_doc_comments
- type_annotate_public_apis
# - unnecessary_brace_in_string_interps
always_declare_return_types: true
Copy link
Member

Choose a reason for hiding this comment

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

Why a map here and no the (more conventional) list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because there is no way to say "false" in a list (unless there's something I'm missing), to override something in recommended/core. If I want to say false later I will have to convert to a map, so just do it now.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. There's a note about this short-coming in the docs:

https://dart.dev/guides/language/analysis-options#disabling-individual-rules

Unfortunate.

annotate_overrides: true
avoid_dynamic_calls: true
avoid_single_cascade_in_expression_statements: true
avoid_unused_constructor_parameters: true
avoid_init_to_null: true
directives_ordering: true
no_adjacent_strings_in_list: true
package_api_docs: true
prefer_final_fields: true
prefer_initializing_formals: true
prefer_void_to_null: true
slash_for_doc_comments: true
type_annotate_public_apis: true
# Work in progress canonical score lints
- avoid_empty_else
- avoid_relative_lib_imports
- avoid_shadowing_type_parameters
- await_only_futures
- camel_case_extensions
- camel_case_types
- curly_braces_in_flow_control_structures
- empty_catches
- file_names
- hash_and_equals
- iterable_contains_unrelated_type
- list_remove_unrelated_type
- no_duplicate_case_values
# - non_constant_identifier_names
- package_prefixed_library_names
- prefer_generic_function_type_aliases
- prefer_is_empty
- prefer_is_not_empty
- prefer_iterable_whereType
- prefer_typing_uninitialized_variables
- provide_deprecation_message
- unawaited_futures
- unnecessary_overrides
- unrelated_type_equality_checks
- valid_regexps
- void_checks
unawaited_futures: true
58 changes: 16 additions & 42 deletions analysis_options_presubmit.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Change analysis_options.yaml and analysis_options_presubmit.yaml
# together.
include: package:pedantic/analysis_options.1.11.0.yaml
include: package:lints/recommended.yaml

analyzer:
errors:
Expand All @@ -22,45 +22,19 @@ analyzer:
- 'testing/test_package_export_error/**'
linter:
rules:
- always_declare_return_types
- avoid_dynamic_calls
- avoid_single_cascade_in_expression_statements
- avoid_unused_constructor_parameters
- annotate_overrides
- avoid_init_to_null
- directives_ordering
- no_adjacent_strings_in_list
- package_api_docs
- prefer_final_fields
- prefer_initializing_formals
- prefer_void_to_null
- slash_for_doc_comments
- type_annotate_public_apis
# - unnecessary_brace_in_string_interps
always_declare_return_types: true
annotate_overrides: true
avoid_dynamic_calls: true
avoid_single_cascade_in_expression_statements: true
avoid_unused_constructor_parameters: true
avoid_init_to_null: true
directives_ordering: true
no_adjacent_strings_in_list: true
package_api_docs: true
prefer_final_fields: true
prefer_initializing_formals: true
prefer_void_to_null: true
slash_for_doc_comments: true
type_annotate_public_apis: true
# Work in progress canonical score lints
- avoid_empty_else
- avoid_relative_lib_imports
- avoid_shadowing_type_parameters
- await_only_futures
- camel_case_extensions
- camel_case_types
- curly_braces_in_flow_control_structures
- empty_catches
- file_names
- hash_and_equals
- iterable_contains_unrelated_type
- list_remove_unrelated_type
- no_duplicate_case_values
# - non_constant_identifier_names
- package_prefixed_library_names
- prefer_generic_function_type_aliases
- prefer_is_empty
- prefer_is_not_empty
- prefer_iterable_whereType
- prefer_typing_uninitialized_variables
- provide_deprecation_message
- unawaited_futures
- unnecessary_overrides
- unrelated_type_equality_checks
- valid_regexps
- void_checks
unawaited_futures: true
4 changes: 2 additions & 2 deletions lib/dartdoc.dart
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ class Dartdoc {
var indexPath = path.joinAll([origin, 'index.html']);
var file = config.resourceProvider.getFile(fullPath);
if (!file.exists) {
return null;
return;
}
var decoder = JsonDecoder();
List<Object> jsonData = decoder.convert(file.readAsStringSync());
Expand Down Expand Up @@ -415,7 +415,7 @@ class Dartdoc {
// Remove so that we properly count that the file doesn't exist for
// the orphan check.
visited.remove(fullPath);
return null;
return;
}
visited.add(fullPath);
var stringLinks = stringLinksAndHref.item1;
Expand Down
7 changes: 3 additions & 4 deletions lib/src/comment_references/parser.dart
Original file line number Diff line number Diff line change
Expand Up @@ -131,13 +131,12 @@ class CommentReferenceParser {
var typeVariablesResult = _parseTypeVariables();
if (typeVariablesResult.type == _TypeVariablesResultType.endOfFile) {
break;
} else if (typeVariablesResult.type ==
_TypeVariablesResultType.notTypeVariables) {
// Do nothing, _index has not moved.
;
} else if (typeVariablesResult.type ==
_TypeVariablesResultType.parsedTypeVariables) {
children.add(typeVariablesResult.node);
} else {
assert(typeVariablesResult.type ==
_TypeVariablesResultType.notTypeVariables);
}
}
if (_atEnd || _thisChar != $dot) {
Expand Down
16 changes: 11 additions & 5 deletions lib/src/dartdoc_options.dart
Original file line number Diff line number Diff line change
Expand Up @@ -556,7 +556,9 @@ abstract class DartdocOption<T extends Object> {
/// Apply the function [visit] to [this] and all children.
void traverse(void Function(DartdocOption option) visit) {
visit(this);
_children.values.forEach((d) => d.traverse(visit));
for (var value in _children.values) {
value.traverse(visit);
}
}
}

Expand Down Expand Up @@ -731,7 +733,7 @@ class DartdocOptionSet extends DartdocOption<void> {

/// [DartdocOptionSet] always has the null value.
@override
void valueAt(Folder dir) => null;
void valueAt(Folder dir) {}

/// Since we have no value, [_onMissing] does nothing.
@override
Expand All @@ -742,7 +744,9 @@ class DartdocOptionSet extends DartdocOption<void> {
/// configuration object.
@override
void traverse(void Function(DartdocOption option) visitor) {
_children.values.forEach((d) => d.traverse(visitor));
for (var value in _children.values) {
value.traverse(visitor);
}
}
}

Expand Down Expand Up @@ -1033,7 +1037,9 @@ abstract class _DartdocFileOption<T> implements DartdocOption<T> {
}
}
}
canonicalPaths.forEach((p) => _yamlAtCanonicalPathCache[p] = yamlData);
for (var canonicalPath in canonicalPaths) {
_yamlAtCanonicalPathCache[canonicalPath] = yamlData;
}
return yamlData;
}
}
Expand Down Expand Up @@ -1137,7 +1143,7 @@ abstract class _DartdocArgOption<T> implements DartdocOption<T> {
/// 'something-bar-over-the-hill' (with default skip).
/// This allows argument names to reflect nested structure.
static String _keysToArgName(Iterable<String> keys, [int skip = 1]) {
var argName = "${keys.skip(skip).join('-')}";
var argName = keys.skip(skip).join('-');
argName = argName.replaceAll('_', '-');
// Do not consume the lowercase character after the uppercase one, to handle
// two character words.
Expand Down
1 change: 1 addition & 0 deletions lib/src/experiment_options.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
library dartdoc.experiment_options;

import 'package:analyzer/file_system/file_system.dart';
// ignore: implementation_imports
import 'package:analyzer/src/dart/analysis/experiments.dart';
import 'package:dartdoc/src/dartdoc_options.dart';

Expand Down
8 changes: 4 additions & 4 deletions lib/src/generator/empty_generator.dart
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@ import 'package:dartdoc/src/model_utils.dart';
/// it were.
class EmptyGenerator extends Generator {
@override
Future<void> generate(PackageGraph _packageGraph, FileWriter writer) {
logProgress(_packageGraph.defaultPackage.documentationAsHtml);
for (var package in {_packageGraph.defaultPackage}
..addAll(_packageGraph.localPackages)) {
Future<void> generate(PackageGraph packageGraph, FileWriter writer) {
logProgress(packageGraph.defaultPackage.documentationAsHtml);
for (var package in {packageGraph.defaultPackage}
..addAll(packageGraph.localPackages)) {
for (var category in filterNonDocumented(package.categories)) {
logProgress(category.documentationAsHtml);
}
Expand Down
2 changes: 1 addition & 1 deletion lib/src/generator/html_generator.dart
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ class HtmlGeneratorBackend extends DartdocGeneratorBackend {
}

Future<void> _copyResources(FileWriter writer) async {
for (var resourcePath in resources.resource_names) {
for (var resourcePath in resources.resourceNames) {
if (!resourcePath.startsWith(_dartdocResourcePrefix)) {
throw StateError('Resource paths must start with '
'$_dartdocResourcePrefix, encountered $resourcePath');
Expand Down
4 changes: 2 additions & 2 deletions lib/src/generator/html_resources.g.dart
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// WARNING: This file is auto-generated. Do not taunt.
Copy link
Member

Choose a reason for hiding this comment

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

Do not taunt!

Ha!

// WARNING: This file is auto-generated. Do not edit.

const List<String> resource_names = [
const List<String> resourceNames = [
'package:dartdoc/resources/favicon.png',
'package:dartdoc/resources/github.css',
'package:dartdoc/resources/highlight.pack.js',
Expand Down
1 change: 1 addition & 0 deletions lib/src/generator/templates.aot_renderers_for_html.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
// the variable is not used; generally when the section is checking if a
// non-bool, non-Iterable field is non-null.
// ignore_for_file: unused_local_variable
// ignore_for_file: non_constant_identifier_names, unnecessary_string_escapes

import 'dart:convert' as _i19;

Expand Down
1 change: 1 addition & 0 deletions lib/src/generator/templates.aot_renderers_for_md.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
// the variable is not used; generally when the section is checking if a
// non-bool, non-Iterable field is non-null.
// ignore_for_file: unused_local_variable
// ignore_for_file: non_constant_identifier_names, unnecessary_string_escapes

import 'dart:convert' as _i19;

Expand Down
1 change: 1 addition & 0 deletions lib/src/generator/templates.runtime_renderers.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
// files in the tool/mustachio/ directory.

// ignore_for_file: camel_case_types, deprecated_member_use_from_same_package
// ignore_for_file: non_constant_identifier_names, unnecessary_string_escapes
// ignore_for_file: unused_import
import 'package:dartdoc/src/element_type.dart';
import 'package:dartdoc/src/generator/template_data.dart';
Expand Down
6 changes: 4 additions & 2 deletions lib/src/io_utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ import 'dart:io' as io;

import 'package:analyzer/file_system/file_system.dart';
import 'package:analyzer/file_system/physical_file_system.dart';
import 'package:analyzer/src/generated/sdk.dart';
import 'package:analyzer/src/test_utilities/mock_sdk.dart';
// ignore: implementation_imports
import 'package:analyzer/src/generated/sdk.dart' show SdkLibrary;
// ignore: implementation_imports
import 'package:analyzer/src/test_utilities/mock_sdk.dart' show MockSdkLibrary;
import 'package:path/path.dart' as path show Context;

Encoding utf8AllowMalformed = Utf8Codec(allowMalformed: true);
Expand Down
2 changes: 1 addition & 1 deletion lib/src/logging.dart
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ void startLogging(LoggingContext config) {
// the backspace to occur for stderr as well.
stderr.write('${ansi.backspace} ${ansi.backspace}');
}
stderr.writeln('$message');
stderr.writeln(message);
}
writingProgress = false;
});
Expand Down
4 changes: 2 additions & 2 deletions lib/src/markdown_processor.dart
Original file line number Diff line number Diff line change
Expand Up @@ -284,9 +284,9 @@ void showWarningsForGenericsOutsideSquareBracketsBlocks(
PackageWarningMode.ignore) {
for (var position in findFreeHangingGenericsPositions(text)) {
var priorContext =
'${text.substring(max(position - maxPriorContext, 0), position)}';
text.substring(max(position - maxPriorContext, 0), position);
var postContext =
'${text.substring(position, min(position + maxPostContext, text.length))}';
text.substring(position, min(position + maxPostContext, text.length));
priorContext = priorContext.replaceAll(allBeforeFirstNewline, '');
postContext = postContext.replaceAll(allAfterLastNewline, '');
var errorMessage = '$priorContext$postContext';
Expand Down
1 change: 1 addition & 0 deletions lib/src/model/accessor.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import 'package:analyzer/dart/element/element.dart';
import 'package:analyzer/source/line_info.dart';
// ignore: implementation_imports
import 'package:analyzer/src/dart/element/member.dart' show ExecutableMember;
import 'package:dartdoc/src/element_type.dart';
import 'package:dartdoc/src/model/comment_referable.dart';
Expand Down
9 changes: 5 additions & 4 deletions lib/src/model/comment_referable.dart
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,11 @@ extension CommentReferableEntryGenerators on Iterable<CommentReferable> {
extension CommentReferableEntryBuilder on Map<String, CommentReferable> {
/// Like [Map.putIfAbsent] except works on an iterable of entries.
void addEntriesIfAbsent(
Iterable<MapEntry<String, CommentReferable>> entries) =>
entries.forEach((e) {
if (!containsKey(e.key)) this[e.key] = e.value;
});
Iterable<MapEntry<String, CommentReferable>> entries) {
for (var entry in entries) {
if (!containsKey(entry.key)) this[entry.key] = entry.value;
}
}
}

/// Support comment reference lookups on a Nameable object.
Expand Down
6 changes: 3 additions & 3 deletions lib/src/model/documentation_comment.dart
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ mixin DocumentationComment

/// Matches YouTube IDs from supported YouTube URLs.
static final _validYouTubeUrlPattern =
RegExp('https://www\.youtube\.com/watch\\?v=([^&]+)\$');
RegExp(r'https://www\.youtube\.com/watch\?v=([^&]+)$');

/// An argument parser used in [_injectYouTube] to parse a `{@youtube}`
/// directive.
Expand Down Expand Up @@ -736,14 +736,14 @@ mixin DocumentationComment
firstOfPair.add(results[i]);
}
}
firstOfPair.forEach((element) {
for (var element in firstOfPair) {
final result = element.group(2).trim();
if (result.isEmpty) {
warn(PackageWarning.missingCodeBlockLanguage,
message:
'A fenced code block in Markdown should have a language specified');
}
});
}
}

/// Returns the documentation for this literal element unless
Expand Down
4 changes: 1 addition & 3 deletions lib/src/model/field.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
// BSD-style license that can be found in the LICENSE file.

import 'package:analyzer/dart/element/element.dart';
import 'package:analyzer/src/dart/element/element.dart';
import 'package:dartdoc/src/model/feature.dart';
import 'package:dartdoc/src/model/model.dart';
import 'package:dartdoc/src/render/source_code_renderer.dart';
Expand Down Expand Up @@ -82,8 +81,7 @@ class Field extends ModelElement
/// Returns true if the FieldElement is covariant, or if the first parameter
/// for the setter is covariant.
@override
bool get isCovariant =>
setter?.isCovariant == true || (field as FieldElementImpl).isCovariant;
bool get isCovariant => setter?.isCovariant == true || field.isCovariant;

@override
bool get isFinal {
Expand Down
Loading