Code review comment for lp:~unity-api-team/unity-scopes-api/child-scopes-option

A few comments:

The tutorial needs updating for the new config attribute. It needs to mention that this is an array setting, with semicolon as the separator. It should also say what the attribute is used for. (The scope author needs be aware of the consequences of forgetting to mention a child scope in this list.)

241 + string child_str = parser()->get_string(scope_config_group, child_scope_ids_key);
...
280 +std::vector<std::string> ScopeConfig::parse_scope_ids(std::string const& val)

We don't need this. The IniParser has a method get_string_array() that splits an array attribute into its components automatically. The separator used in .ini files is semicolon (instead of comma). We should use this because it maintains the established syntax for .ini files and also makes all the code in parse_scope_ids() redundant.

It would be good to have test for a zero-element and a single-element array of child scopes too, just to be sure that the code behaves sanely if someone does something silly in their config file.

24 + should be spearated by commas.

There is a typo here: "spearated". Also it needs to say "semicolons" (see above).

Use "must" instead of "should". If we say "should", that suggests that it's OK to use something other than commas. Also, the ini parser preserves whitespace for string array elements, so it would be good to show an example. Something along the lines of:

ChildScopes = com.acme.scopeX;com.acme.scopeY

Somewhat stupidly, the ini parser strips leading whitespace, but not trailing whitespace:

ChildScopes = a;b;c

If there is a single trailing space on that line, the values of the array elements are "a", "b", and "c " :-(

It might be a nice touch to warn about trailing whitespace in the doc.

131 + \brief Return the list of scope identifiers aggregated by this scope.
132 + The list returned by this method comes from the .ini file and it's up

We need a newline after the first sentence, otherwise the entire para becomes part of the summary.

I recommend to avoid contractions ("it's") in the documentation. Contractions are usually shunned in formal documentation. Even better, make it two sentences:

"The list returned by this method is taken from the scope's `.ini` file. The scope author must ensure that it contains all scopes that an aggregator might collect results from."

134 + scopes which are not currently installed and are optional for proper functioning

This should be "scopes *that* are not currently installed" (because this is a defining relative clause.)