AF-1227: Adds merge method and merged constructor to ClassNameBuilder class.#167
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on HipChat: InfoSec Forum. |
Codecov Report
@@ Coverage Diff @@
## master #167 +/- ##
==========================================
+ Coverage 94.45% 94.48% +0.04%
==========================================
Files 34 34
Lines 1601 1612 +11
==========================================
+ Hits 1512 1523 +11
Misses 89 89 |
greglittlefield-wf
left a comment
greglittlefield-wf
left a comment
There was a problem hiding this comment.
Looks good, just some little things!!
lib/src/util/class_names.dart
Outdated
| addFromProps(props); | ||
| } | ||
|
|
||
| /// Creates a new `ClassNameBuilder` with [_classNamesBuffer] and [_blacklistBuffer] merged from [a] and [b] |
There was a problem hiding this comment.
#nit missing period at the end of this sentence.
lib/src/util/class_names.dart
Outdated
| /// Creates a new `ClassNameBuilder` with [_classNamesBuffer] and [_blacklistBuffer] merged from [a] and [b] | ||
| /// | ||
| /// ClassNameBuilder a = new ClassNameBuilder() | ||
| /// ..add('a'); |
There was a problem hiding this comment.
#nit only one-space indent instead of two
|
|
||
| group('merge', () { | ||
| test('returns className merged from second builder instance', (){ | ||
| ClassNameBuilder otherBuilder = new ClassNameBuilder(); |
There was a problem hiding this comment.
#nit for local variables like this, the convention is to omit the typing on the left-hand side (LHS)
See more info here: https://www.dartlang.org/guides/language/effective-dart/design#avoid-type-annotating-initialized-local-variables
| }); | ||
|
|
||
| group('created with .merged() constructor', () { | ||
| test('',(){ |
There was a problem hiding this comment.
#nit formatting, missing whitespace after comma and before curly brace
| }); | ||
|
|
||
| group('merge', () { | ||
| test('returns className merged from second builder instance', (){ |
There was a problem hiding this comment.
The "returns" part of this test description is misleading, since merge is a void and doesn't return anything.
Should be something like:
group('merge, () {
test('merges the className from the provided builder instance', (){
lib/src/util/class_names.dart
Outdated
| /// Merges together the classes and blacklists present in another `ClassNameBuilder` instance. | ||
| /// | ||
| /// Takes [other._blacklistBuffer] and merges it into [_blacklistBuffer] | ||
| /// and takes [other._classNamesBuffer] and merges it into [_classNamesBuffer]. |
There was a problem hiding this comment.
This second section is redundant, since it's just describing the below code; we could probably just remove it.
lib/src/util/class_names.dart
Outdated
|
|
||
|
|
||
|
|
||
| /// Merges together the classes and blacklists present in another `ClassNameBuilder` instance. |
There was a problem hiding this comment.
This should use a reference to [other].
#nit "ClassNameBuilder instance" is redundant: See https://www.dartlang.org/guides/language/effective-dart/documentation#avoid-redundancy-with-the-surrounding-context
Also, "merges together" is a little vague. What about:
/// Merges the classes and blacklists from [other] into this builder.
lib/src/util/class_names.dart
Outdated
| _blacklistBuffer.write(other._blacklistBuffer); | ||
|
|
||
| if (_classNamesBuffer == null) { | ||
| _classNamesBuffer = new StringBuffer(); |
There was a problem hiding this comment.
This is always initialized (resulting in this line missing test coverage), so this case can probably be removed.
| test('returns blacklist merged from second builder instance', (){ | ||
| ClassNameBuilder otherBuilder = new ClassNameBuilder(); | ||
|
|
||
| builder |
There was a problem hiding this comment.
Since these are only single setter usages and there's no use case for the returned instance of ClassNameBuilder that the cascade provides, it would be the same functionality and more terse to do:
builder.blacklist('a-blacklist');
otherBuilder.blacklist('b-blacklist');| test('returns classname and blacklist merged from second builder instance', (){ | ||
| ClassNameBuilder otherBuilder = new ClassNameBuilder(); | ||
|
|
||
| builder |
There was a problem hiding this comment.
| ClassNameBuilder otherBuilder = new ClassNameBuilder(); | ||
|
|
||
| builder | ||
| ..add('a'); |
There was a problem hiding this comment.
| expect(builder.toClassName(), equals('a b')); | ||
| }); | ||
|
|
||
| test('returns blacklist merged from second builder instance', (){ |
There was a problem hiding this comment.
#nit needs a space after the parenthesis: (){ => () {
There was a problem hiding this comment.
Occurs a couple other places in this file as well
| otherBuilder.add('b'); | ||
|
|
||
| builder.merge(otherBuilder); | ||
| expect(builder.toClassName(), equals('a b')); |
There was a problem hiding this comment.
@kealjones-wk you don't have to change this, but equals is the default Matcher for the second arg of expect, so it can be omitted. Like I said - you definitely do not have to change this, as we are certainly not consistent with this in any of our repos... but I thought I'd point it out.
expect(builder.toClassName(), 'a b');
|
QA +1
Merging. |
7 participants
AF-1227
Ultimate problem:
As a consumer, I should be able to easily merge together the classes and blacklists present in two ClassNameBuilders.
How it was fixed:
Added
mergemethod toClassNameBuilderclass.Added
mergedconstructor toClassNameBuilderclass.Testing suggestions:
Run the unit tests they should pass.
Potential areas of regression:
None.