D15521: Add validator for lists of IP addressesAdded as separate review per comment from Pino onreview D15093. This code will not compile withoutthe updated code in review D15520. Also includesunit test.

[Bruce Anderson]

andersonbruce added inline comments.

INLINE COMMENTS

> jgrulich wrote in simpleiplistvalidator.cpp:39
> In both validator constructors (mean also for IPv6 one), you can directly pass "style" param from contructor, given both enums seem to be identical.

They are the same now but initially they were not (I just added the WithPort option to IPv6) and my thought was that they may not necessarily stay identical in the future. If someone goes in and changes, say, the IPv4 version without realizing it affects the list validator, this code still works.

> pino wrote in simpleiplistvalidator.cpp:62
> no need for a regexp, please use `QString::split()` with `QString::SkipEmptyParts`

I used the regex so that it would split on commas with or without spaces around them and how I read SkipEmptyParts is that it will drop something between two adjacent commas which is not what I was trying to do. 
Now I know that I could use "trimmed()" on the resulting strings to strip off any remaining spaces but I don't see any advantage to either using "trimmed()" every time I use one of the resulting strings or doing it once and creating another local copy of the strings for this purpose. It seemed to me that doing everything in one step was a just as viable approach unless there is something inherently bad with splitting on a regex that I am not aware of, especially given that I commented it to explain what it is doing.

> pino wrote in simpleiplistvalidator.cpp:67
> set (later on), but never use

I don't understand this comment. 'result' is set here to Acceptable, it can be modified to Intermediate at line 102 but if all the addresses are valid it needs to have been set initially to Acceptable for the return at line 106 to be correct.

> pino wrote in simpleiplisttest.cpp:53
> `QVERIFY(foo == bar)` -> `QCOMPARE(foo, bar)`, it applies to all the checks in this file

Any particular reason you think this is superior?

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D15521

To: andersonbruce, jgrulich, pino
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20180917/e49e866a/attachment.html>