-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Output mode for printing playlists of similar tracks #51
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @iammordaty, thank you for the PR and sorry for the late reply! This looks useful indeed, and implementing it as a string option would enable other formats in the future (such as a CSV). I left a few comments in the code. Let me know what you think.
float d = track_idx[i].second; | ||
|
||
pl << "track-id: " << j << ", track-distance: " << d | ||
<< ", track-origin: " << tracks_files[j] << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in principle! I think we should omit the track-
prefixes, though. And track-origin
could just become filename
? And I'm a bit unsure if the track id is useful, since users do not have a way to set it anyway. Would you have a use case in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should omit the
track-
prefixes, though. And track-origin could just become filename?
I thought about consistency with the 'list files' mode, but if there is no such need, I will remove track-
prefixes and change track-origin
to filename
- it clearly indicates what it is and just looks better :-)
And I'm a bit unsure if the track id is useful, since users do not have a way to set it anyway. Would you have a use case in mind?
Thank you for pointing this out, I had doubts about adding it.
As above, I added this field to be consistent with file listing mode. I noticed that this is not a "real" track identifier, but I left it at MR to talk about it to make sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @f0k! Never mind, thanks for the answer and the code review!
f5932bc
to
b391ce1
Compare
Hi, @f0k! Is there any chance of accepting this MR? If I should change anything else, please let me know. Thanks! |
Hello! In musly command line tool, I added the ability to change output mode. The new mode allows printing a playlist of similar tracks in a similar way as when listing files from the collection.
Please see an example outputs below: