Conversation
DamienCassou
left a comment
DamienCassou
left a comment
There was a problem hiding this comment.
Thank you for your work. I think some parts of the PR are not completely related. If you don't agree, can you please tell me why they are? We should add some unit tests in this case if possible.
libmpdel.el
Outdated
| (cl-defmethod libmpdel-list ((_entity (eql albums)) function) | ||
| "Call FUNCTION with all albums as parameter." | ||
| (libmpdel-send-command | ||
| ;; TODO: maybe compute all albums by listing all songs |
There was a problem hiding this comment.
What about creating an ISSUE instead of adding this TODO? This way we can have a conversation about it, make a decision and keep a trace of it.
libmpdel.el
Outdated
| "Return a list of songs from DATA, a server's response." | ||
| (mapcar #'libmpdel--create-song-from-data (libmpdel-group-data data))) | ||
|
|
||
| (defun libmpdel--create-album-from-data (album-data) |
There was a problem hiding this comment.
ALBUM-DATA seems to contain song data instead. If I'm correct, should we rename the variable to song-data?
| (lambda (album-name) | ||
| (libmpdel--album-create :name album-name | ||
| :artist libmpdel--unknown-artist)) | ||
| (libmpdel-sorted-entries data 'Album)))))) |
There was a problem hiding this comment.
before we merge this PR, it would be good to have a PR ready to be merged that introduce sorting back into the UI.
There was a problem hiding this comment.
based on my tests, the output of list album is already alphabetically sorted, but that is not documented - is that good enough?
test/libmpdel-test.el
Outdated
| (let ((song (libmpdel--create-song-from-data | ||
| '((Title . "The song") | ||
| (file . "foo/song.ogg") | ||
| (Date . "1970-01-01") |
There was a problem hiding this comment.
can we pick another date than Unix epoch?
| :album (libmpdel--album-create | ||
| :name (cdr (assq 'Album song-data)) | ||
| :artist (libmpdel--artist-create :name (cdr (assq 'Artist song-data)))) | ||
| :album (libmpdel--create-album-from-data song-data) |
There was a problem hiding this comment.
this seems unrelated to "Add date field to albums". Can we keep that for another PR? I will better understand the goal this way and we can merge this one sooner.
There was a problem hiding this comment.
It is technically unrelated to the field itself, but imo intimately related to the code: adding date fields to albums (and populating those fields with dates) means that the exact same album-creation-code would have to be duplicated in three separate places, to prevent that I factored said code into a function
(especially since in the next PR I'll add even more code to libmpdel--create-album-from-data, which would otherwise also have to be copied identically to three different places)
There was a problem hiding this comment.
I understand, thank you
pinoaffe
force-pushed
the
add-date
branch
2 times, most recently
from
ce0c4f2 to
c7f7ce4
Compare
|
When ready, please click the double-arrow next to my name: This will add the review to my todo list. Feel free to do the same for all PRs you open when ready to get a review. |
|
Can you please rebase this PR? |
DamienCassou
left a comment
DamienCassou
left a comment
There was a problem hiding this comment.
Some of my comments didn't get answered. It's ok not to agree but it would be nice to answer so I better understand your intent.
Oh sorry, I tried to answer several questions at once, for the sake of clarity I will answer each question on its own |
DamienCassou
left a comment
DamienCassou
left a comment
There was a problem hiding this comment.
Great job, thank you!
| :album (libmpdel--album-create | ||
| :name (cdr (assq 'Album song-data)) | ||
| :artist (libmpdel--artist-create :name (cdr (assq 'Artist song-data)))) | ||
| :album (libmpdel--create-album-from-data song-data) |
There was a problem hiding this comment.
I understand, thank you
2 participants
No description provided.