[WIP] Adds MusicBrainz IDs optionally#27
Conversation
luiseduardobrito
changed the title
|
Found a bug in pagination handling, I'll update with a fix soon. Updated the PR with the [WIP] prefix to prevent merging. |
benfoxall
left a comment
benfoxall
left a comment
There was a problem hiding this comment.
Hey @luiseduardobrito! Thank you so much for raising this PR. It's looking great!
I've got a single change: it looks like the headers might be repeated throughout the generated csv file. Other than that, I've added a couple of comments & suggestions.
Thanks again!
| if(includeMBID) { | ||
| return row(headers, d) | ||
| } | ||
| return row(headers, d) |
There was a problem hiding this comment.
The if statement doesn't seem necessary here now that headers is pulled out above.
| if(includeMBID) { | |
| return row(headers, d) | |
| } | |
| return row(headers, d) | |
| return row(headers, d) |
| return row(['artist', 'album', 'name', 'date'], d) | ||
| }) | ||
| .map(csv).join('\n') + '\n']); | ||
| includeHeaders ? (headers.join(',') + '\n' + r) : r |
There was a problem hiding this comment.
🤔 will this add the headers for every page of the output?
It might be easier to add the headers at the download handler instead:
Line 144 in 4d87805
⬆️ , could be something along the lines of ⬇️
const header = includeHeaders ? [headers.join(',') + '\n'] : [];
var b = new Blob(header.concat(data), {type: 'text/csv'})(though maybe there's a neater way to do it…)
| if (mbid) { | ||
| obj[`${child.tagName}_mbid`] = mbid; | ||
| } | ||
| } |
| <p class="help-block">(milliseconds, default 0) how long to wait between making requests</p> | ||
| </div> | ||
| <div class="form-group form-inline"> | ||
| <input type="checkbox" class="form-control" name="lastfm-include-headers" id="lastfm-include-headers" checked="false"> |
There was a problem hiding this comment.
(just a suggestion, feel free to ignore)
It may be possible to use Rivets rv-checked, to automatically bind this value.
| <input type="checkbox" class="form-control" name="lastfm-include-headers" id="lastfm-include-headers" checked="false"> | |
| <input type="checkbox" class="form-control" name="lastfm-include-headers" rv-checked="include_headers"> |
Then in the code later you could access this value through the state:
var includeHeaders = state.include_headers;……… However, I've no idea what version of Rivets this is using, and if there are any issues with the different options, so feel free to ignore this and I could take a look later.
2 participants
Resolves #23