----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105306/#review14914 -----------------------------------------------------------
Ship it! Good and correct test, but my nitpicking clone is somehow active today: tests/core/playlists/TestPlaylistFormat.h <http://git.reviewboard.kde.org/r/105306/#comment11707> Code style: this should be indented to 8 spaces accorting to intro_and_style.txt. (not a biggie for sure) tests/core/playlists/TestPlaylistFormat.cpp <http://git.reviewboard.kde.org/r/105306/#comment11708> Just for future: you don't have to declare and define constructor at all if it is empty. (at least I hope) tests/core/playlists/TestPlaylistFormat.cpp <http://git.reviewboard.kde.org/r/105306/#comment11709> Nitpicking: while I for sure agree that defining variables for values that could have been defined in-line is better for clarity, this case is too obvious IMO and calling just url.setFilename( "something" ); would make this a bit shorter. ;) tests/core/playlists/TestPlaylistFormat.cpp <http://git.reviewboard.kde.org/r/105306/#comment11710> Just for future: if some methods are such tightly coupled as Playlists::getFormat() and Playlists::isPlaylist() I think it is okay to test them in just one method to make it shorter. But Sven and others may have different opinion, disregard this message in that case. - Matěj Laitl On June 20, 2012, 2:17 p.m., Jasneet Bhatti wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/105306/ > ----------------------------------------------------------- > > (Updated June 20, 2012, 2:17 p.m.) > > > Review request for Amarok, Bart Cerneels, Matěj Laitl, and Sven Krohlas. > > > Description > ------- > > Adds a unit test for core/playlists/PlaylistFormat > > > Diffs > ----- > > tests/core/CMakeLists.txt 8f7db22 > tests/core/playlists/CMakeLists.txt PRE-CREATION > tests/core/playlists/TestPlaylistFormat.h PRE-CREATION > tests/core/playlists/TestPlaylistFormat.cpp PRE-CREATION > > Diff: http://git.reviewboard.kde.org/r/105306/diff/ > > > Testing > ------- > > Compiles, links and runs fine. > > > Thanks, > > Jasneet Bhatti > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
