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

fix: format-comment is listing the macros from dart doc. #823

Merged
merged 4 commits into from May 3, 2022

Conversation

vlkonoshenko
Copy link
Member

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[X] Bug fix
[ ] New rule
[ ] Changes an existing rule
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

#812

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

@vlkonoshenko vlkonoshenko self-assigned this Apr 30, 2022
@github-actions
Copy link

github-actions bot commented Apr 30, 2022

Dart Code Metrics unused files report of dart_code_metrics. ✅

Summary

  • Scanned package folders: bin, example, lib
  • No unused files found! ✅

@github-actions
Copy link

github-actions bot commented Apr 30, 2022

Dart Code Metrics analyze report of dart_code_metrics. ✅

Summary

  • Scanned folders: bin, example, lib, test

  • Total scanned files: 424

  • Total lines of source code: 7446

  • Total classes: 290

  • Average Cyclomatic Number per line of code: 0.35

  • Average Source Lines of Code per method: 6

  • Total tech debt: 1318.0 hours

  • Found issues: 6 ⚠

@codecov
Copy link

codecov bot commented Apr 30, 2022

Codecov Report

Merging #823 (4c6fa5d) into master (ded9d0e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #823   +/-   ##
=======================================
  Coverage   87.32%   87.32%           
=======================================
  Files         279      279           
  Lines        5931     5932    +1     
=======================================
+ Hits         5179     5180    +1     
  Misses        752      752           
Impacted Files Coverage Δ
...lyzer/rules/rules_list/format_comment/visitor.dart 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ded9d0e...4c6fa5d. Read the comment docs.

Comment on lines 50 to 51
final regTemplateExp = RegExp(r"{@template [\w'-]+}");
final regMacroExp = RegExp(r"{@macro [\w'-]+}");
Copy link
Contributor

@ookami-kb ookami-kb Apr 30, 2022

Choose a reason for hiding this comment

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

I would make them file-private final variables. It can look like a micro-optimization, but for analyzer running on thousands comments that could matter.

E.g. this benchmark:

import 'package:benchmark/benchmark.dart';

void main() {
  benchmark('Create RegExp', () {
    List.generate(100000, (i) {
      final x = RegExp(r"{@template [\w'-]+}");
      return x.hasMatch('abc');
    });
  });

  benchmark('Reuse RegExp', () {
    List.generate(100000, (i) {
      return _regex.hasMatch('abc');
    });
  });
}

final _regex = RegExp(r"{@template [\w'-]+}");

gives such a result:

❯ dart run benchmark --dir=.
 DONE  ././lib/example_benchmark.dart (5 s)
 ✓ Create RegExp (103 ms)
 ✓ Reuse RegExp (22 ms)

Benchmark suites: 1 passed, 1 total
Benchmarks:       2 passed, 2 total
Time:             5 s
Ran all benchmark suites.

Copy link
Member

Choose a reason for hiding this comment

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

Nice catch, actually!

Comment on lines 8 to 10
final _regTemplateExp = RegExp(r"{@template [\w'-]+}");
final _regMacroExp = RegExp(r"{@macro [\w'-]+}");

Copy link
Contributor

Choose a reason for hiding this comment

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

Most of the examples I've seen online also use . in the template names. And this is supported by dartdoc.

ex:

/// {@template my_project.my_class.my_method}

Wouldn't this be better?

Suggested change
final _regTemplateExp = RegExp(r"{@template [\w'-]+}");
final _regMacroExp = RegExp(r"{@macro [\w'-]+}");
final _regTemplateExp = RegExp(r"{@template [\w'-\.]+}");
final _regMacroExp = RegExp(r"{@macro [\w'-\.]+}");

Copy link
Contributor

Choose a reason for hiding this comment

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

In general, it can even be combined and simplified to r'{@(template|macro) .+}'.

I don't see any actual profit in making it stricter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Done))

@dkrutskikh dkrutskikh merged commit 07a2bfd into master May 3, 2022
@dkrutskikh dkrutskikh deleted the 812_format_comment_issue branch May 3, 2022 16:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants