Album filter support - needs work #63
Conversation
There was a problem hiding this comment.
@HenrikHoyer thanks for the contribution. Overall it is correct, I just added some comments. However, I'd put the filter in the func (w *Worker) albums() function, so that you're filtering out the albums before adding them to the worker pipeline. What do you think?
Using the prefix is a good start, although a more powerful tool are the regular expressions. It can be challenging for the users to write them, but they can solve problems that are impossible to solve with just the prefix. In case you want to give it a try, take a look at Regexp.Match
| @@ -115,6 +116,7 @@ func (w *Worker) saveImage(image albumImage, folder string) error { | |||
| return errors.New("unable to find valid image filename, skipping") | |||
| } | |||
| dest := fmt.Sprintf("%s/%s", folder, image.Name()) | |||
| dest = strings.Replace(dest, "?", "_", -1) | |||
There was a problem hiding this comment.
this is not related to the current PR. It is, instead, related to #62
It's better to split the work, so that each one can have its own discussion
| @@ -224,6 +231,13 @@ func (w *Worker) albumWorker(id int) { | |||
| log.Debugf("Quitting albumWorker %d", id) | |||
| return | |||
| } | |||
|
|
|||
| filter := w.cfg.AlbumFilter | |||
| if len(filter) > 0 && !strings.HasPrefix(strings.ToLower(album.URLPath), strings.ToLower(filter)) { // Improve: use a real 'mathces' function instead of just HasPrefix | |||
There was a problem hiding this comment.
since filter is a string, maybe using if string != "" is a bit more clear
There was a problem hiding this comment.
using lowercase here makes the strong assumption that the user wants a case-insensitive filter. Maybe this is the correct thing, maybe not. Using a regexp match can be more powerful here, although a little bit more expensive
|
Ah, I forgot to mention tests: they're definitely useful if you're not expert with go and/or regexp, I'd add a couple of them to ensure you're filtering out the correct strings |
❓ What
Added an
Album_filterconfiguration option to download only a subset of all albums.This is my first time using Go, and I therefore need assistance for improving/completing this PR.
I have jut used
strings.HasPrefix()for the match, was is the preferred way for filtering?I haven't made any tests for the code, is it needed?