Closed AdamWHY2K closed 4 weeks ago
Looks great, will need to review it more later today when I have time but at a glace I see a few small things. Maybe number_of_tracks_to_grab
should be number_of_albums_to_grab
right? Tracks makes me think of songs. Looking at it now maybe these two options could be combined into only number_of_albums_to_grab
.
Options could simply be something like:
all
first_page
increment_page
50
Where 50 is just any number of albums. Would need to parse the option to see if it is a string or a number but it shouldn't be too hard. And for a default I think you are right with a default of 10. Let me know what you think. Sorry if I rambled!
agree with the var name changes.
not sure about combining number_of_albums_to_grab
and search_type
though as then i don't think it'd be possible for a user to, for example, use incrementing search but also have each search look for more than 10 albums at a time
Good point about combining the options. PR looks great I will test soon and try and merge for an update tonight 👍
Awesome merging!
users now specify which
search_type
they want to use, choices are:also added
number_of_tracks_to_grab
which allows users to specify how many albums to grab at once (unless using search_type all)search type defaults to first_wanted_page number_of_tracks_to_grab defaults to 10
this pr should close out https://github.com/mrusse/soularr/issues/12 now i think