Diff Detail
- Repository
- R241 KIO
- Lint
Lint Skipped - Unit
Unit Tests Skipped
| src/filewidgets/kfileplacesmodel.cpp | ||
|---|---|---|
| 360 | Please double-check indentation, looks like 5 spaces. | |
| 859 | remove one of the two spaces after '=' Add const in front, while at it. | |
| 860 | Please swap these two arguments around. In all preceding if()s, you have new != old, while here it's old != new, makes reading more difficult. | |
| src/filewidgets/kfileplacesmodel.h | ||
| 139 | Missing method documentation, including @since 5.41. | |
the new API and functionality should be covered by tests, it isn't so far as far as I can see.
This seems to be bundling several unrelated additions into the same commit?
Please have one commit per feature / change / addition.
I know that phabricator makes it difficult to have one review per commit, but at least the commits should be separate.
| autotests/kfileplacesmodeltest.cpp | ||
|---|---|---|
| 893 | expectedScheme is part of the expectedUrl already, so I don't see much point in this separate data column and check. | |
| src/filewidgets/kfileplacesmodel.cpp | ||
| 933 | This would fit better in the if/else above. | |
| 935 | Can you explain this condition? I understand the idea of an early exit if nothing to do, but I'm surprised that two different values of before would lead to "nothing to do". | |
| src/filewidgets/kfileplacesmodel.h | ||
| 52 | ///< @since 5.41 | |
| src/filewidgets/kfileplacesmodel.cpp | ||
|---|---|---|
| 913 | This should get its own unittest, especially since there are lots of edge cases. | |