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. | |
882 | remove one of the two spaces after '=' Add const in front, while at it. | |
883 | 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 | ||
133 | 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 ↗ | (On Diff #22703) | 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 | ||
951 ↗ | (On Diff #22651) | This would fit better in the if/else above. |
953 ↗ | (On Diff #22651) | 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 ↗ | (On Diff #22651) | ///< @since 5.41 |
src/filewidgets/kfileplacesmodel.cpp | ||
---|---|---|
931 ↗ | (On Diff #22651) | This should get its own unittest, especially since there are lots of edge cases. |