strongest lint rules, fixed all lint and dart 2 warnings #33
Conversation
Module FindingsNo security relevant content detected. Please review for security relevance and request security review as needed. |
pubspec.yaml
Outdated
| @@ -5,7 +5,7 @@ author: Rob Becker <rob.becker@workiva.com> | |||
| homepage: https://github.com/Workiva/font_face_observer | |||
|
|
|||
| environment: | |||
| sdk: '>=1.22.1 <2.0.0' | |||
| sdk: '>=1.22.1 <= 2.0.0' | |||
There was a problem hiding this comment.
allow Dart 2 .. this will need updating to allow > 2.0.0
There was a problem hiding this comment.
nah... I'll wait until a stable build of dart 2 before making this call.
| bool hasEndQuote = family.endsWith('"') || family.endsWith("'"); | ||
| final bool hasStartQuote = | ||
| family.startsWith('"') || family.startsWith("'"); | ||
| final bool hasEndQuote = family.endsWith('"') || family.endsWith("'"); |
| } | ||
| static Future<FontLoadResult> _loadAdobeBlank() => | ||
| new FontFaceObserver(adobeBlankFamily, group: adobeBlankFamily) | ||
| .load(adobeBlankFontBase64Url); |
There was a problem hiding this comment.
prefer arrow functions for single statement return functions
| container | ||
| ..append(_rulerSansSerif.element) | ||
| ..append(_rulerSerif.element) | ||
| ..append(_rulerMonospace.element); |
lib/support.dart
Outdated
| @@ -91,8 +93,9 @@ bool _supportsStretch() { | |||
| bool _supportsNativeFontLoading() { | |||
| bool supports = true; | |||
| try { | |||
| // ignore: unnecessary_statements | |||
There was a problem hiding this comment.
including this ignore because even though this looks like the statement doesn't do anything, we want to try the accessor to trigger an exception in browsers that don't support FontFace API
There was a problem hiding this comment.
Ah, could you add a comment to the test explaining what this document.fonts.status line is for? It wouldn't make immediate sense to me, and I'd be extra confused if I found it following this ignore: unnecessary_statements line.
There was a problem hiding this comment.
Sure! It's similar to the above function _supportsStretch where it tries to use the API and catches the exception that gets thrown if that API isn't available in the browser. I'll add a comment to this effect.
| elements.forEach( (Element el) => print('${el.tagName}.${el.className}')); | ||
| for (Element el in elements) { | ||
| print('${el.tagName}.${el.className}'); | ||
| } |
There was a problem hiding this comment.
replace anonymous forEach with for..in
Current coverage is 88.66% (diff: 93.22%)@@ master #33 diff @@
==========================================
Files 4 4
Lines 294 300 +6
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 263 266 +3
- Misses 31 34 +3
Partials 0 0
|
|
Hmm, using final in all these places seems weird to me. Is it a performance boost or something? I could imagine coming to this code later and wanting to update a local variable, but wondering if that would go against the intention of the author who put final on it. |
| @@ -120,8 +120,7 @@ class Ruler { | |||
|
|
|||
| _collapsible.append(_collapsibleInner); | |||
| _expandable.append(_expandableInner); | |||
| element.append(_collapsible); | |||
| element.append(_expandable); | |||
| element..append(_collapsible)..append(_expandable); | |||
There was a problem hiding this comment.
This... does not seem more readable to me, but 🤷♂️
There was a problem hiding this comment.
dart format usually puts cascades on their own new line .. not sure why it chose to 1-line this one.
| bool _isNullOrWhitespace(String s) { | ||
| return s == null || s.trim().isEmpty; | ||
| } | ||
| bool _isNullOrWhitespace(String s) => s == null || s.trim().isEmpty; |
|
All the finals are because of the prefer_final_fields and prefer_final_locals. They report that the compiler can optimize the code, but it doesn't say exactly what that means.
|
|
+1 |
+10✅ CI Passed |
Quality Review Approval: QA +1
|
Overview
Turned on even more lint rules and fixed lint. Also fixed any Dart 2 warnings using the Dart 2 dev branch. This library should now work seamlessly with Dart 2.
Testing