Skip to content

Add --start parameter#22

Open
lubien wants to merge 4 commits intohenrik:masterfrom
lubien:master
Open

Add --start parameter#22
lubien wants to merge 4 commits intohenrik:masterfrom
lubien:master

Conversation

@lubien
Copy link

@lubien lubien commented Sep 22, 2016

Works for both oldest and latest runnings ;)

@henrik
Copy link
Owner

henrik commented Sep 24, 2016

Hi, thank you so much for this!

I just unbroke the build in master, so if you rebase, the tests should pass.

Some thoughts – let me know what you think.

  • I wonder if something like --start-from or --start-from-episode might be clearer than --start.
  • The README text might be clearer if it was more specific and had an actual example, something like:
You may want to start downloading from a given episode number, like:

     ./sipper --user me@example.com --pw mypassword --oldest-first --start-from-episode 50
  • Using String.contains? to find the given episode seems like it could be fragile. As a fictitious example, what if you wanted to start from episode "100" but episode 30 was titled "030 - accepting 1000 connections". We would then stop at episode 30. Could we be a bit more specific in our string matching?

If you feel like it, a test would be really nice, but I've been lax about it elsewhere in this project, so I don't consider it a must at this point :)

@lubien
Copy link
Author

lubien commented Sep 29, 2016

Sorry for delaying my answer.

Indeed --start-from-episode seems better.

About the docs, you may be the most fit one to do so can you do it? hehe

How about using:

%{"episode" => episode} = Regex.named_captures(~r/^(?<episode>\d{3})/, "123_foo_bar")

@henrik
Copy link
Owner

henrik commented Oct 2, 2016

Sure, happy to fix the docs :) I can do that after merging.

Yeah, anchoring the regex to the beginning of line like that seems like a good idea. Go for it!

lubien added 4 commits October 5, 2016 10:48
* Rename --start parameter to --start-from-episode
* Use Regex to match the first episode and reject the ones before it
@lubien
Copy link
Author

lubien commented Oct 5, 2016

About the tests I'm not sure how I'm going to test the runner with Auth and stuff :x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants