PR for issue #219

[ribbons]

Thanks for the PR. Yes, if you could first rebase this PR against the current master that would make both reviewing and the merge easier (no need to create a new PR, just rebase locally and then force push).

[nbl1268]

Okay have rebased and forced push for this PR.

[ribbons]

Unfortunately I think you merged rather than rebasing. Hope it is okay but I've taken the liberty of rebasing it for you :wink:. You'll need to do something along the lines of the below to get your local repository back in sync:

git fetch origin
git reset --hard origin/master

I've had a very quick skim through the code and it looks like some date capitalisation changes have got caught up in this PR. Could you remove those from this PR and submit them separately?

[nbl1268]

Oh yes, actually the button did say 'merge'... thanks for fixing that for me. Right, yes I did add code to handle date capitalisation, and some tests for that, as a result of seeing those other date variations whilst testing this on some real podcasts. Will remove and raise as new PR.

[nbl1268]

Just read you note re adding some tests. Yes that would be good. Assuming we would create a new module for this (as we aren't testing the textutils).

And the tests would focus around some podcast files with various ProgrammeName and EpisodeName combinations so we can see the results of our 'SmartName' (something like the table you put together).

Programme Name	Episode Name	'Smart' Name
Programme 1	Episode 1	Programme 1: Episode 1
Programme 2	Programme 2: Episode 1	Programme 2: Episode 1
Programme 3	Programme 3 - Episode 1 - 05-12-2017	Programme 3: Episode 1
Programme 4	Episode 1: 05/12/2017	Programme 4: Episode 1
Programme 5	05/12/2017	Programme 5

[ribbons]

Just read you note re adding some tests. Yes that would be good. Assuming we would create a new module for this (as we aren't testing the textutils).

No, you can just add a function to TestTextUtils as the function to be tested is in TextUtils.

And the tests would focus around some podcast files with various ProgrammeName and EpisodeName combinations so we can see the results of our 'SmartName' (something like the table you put together).

Yes, that table of values would be a good starting point. Shouldn't need any files, just pass the values into the function like the date stripping tests.

[nbl1268]

Worked through all feedback items and believe I've got the changes (including some tests) covered off :)

[ribbons]

Are you ready for me to do another review or are there some more changes you'd like to make first?

[nbl1268]

Yes I think so. Had been checking some real podcast downloads to see that we got the expected results in the smartname... so far getting the expected outcome.

[nbl1268]

Also let me know if i should be merging your recent change in to my branch? or leaving this as is for the moment?

[ribbons]

Also let me know if i should be merging your recent change in to my branch? or leaving this as is for the moment?

Yes, that would be great. I've just committed a bump of the xUnit Visual Studio runner as it was causing me issues too, so you can also remove that from this PR now. If you get stuck, let me know. Once that is done that I'll give it another review.

[nbl1268]

Can see the change to packages.config is the xunit.runner.visualstudio is version 2.3.1, so have updated my version to same.

For some reason (that I dont understand) updating xunit has changed the first line of the package.config file even thought it still looks the same to me (assuming there is/was some non-printable character.

So now stuck and dont want to mess this up. If you can let the know the safe way to handle this that would be appreciated.

[ribbons]

Might be fixed now but think it could all do with a rebase, is looking rather a tangle of commits. Have run out of time for that this evening so will do that tomorrow evening.

[nbl1268]

okay have removed unused code and merged your recent updates. Ready for you to recheck.

[ribbons]

Cool, perfect. Am tied up with things until the weekend but will have a comb though then.

[nbl1268]

Thanks feedback appreciated - will work through the suggested changes. Firstly though, assuming I just need to sync my local to pick the changes (additional tests you've added) and then do an update branch (from your master) for the other recent changes (TLS and cancelled download etc).

[ribbons]

No worries, great. Yes, you'll need to pull from GitHub to your local repository and also merge the changes from my master.

[nbl1268]

hmm checks are failing on the BuildEpisodeSmartName function.

.RadioDldTest.TestTextUtils.BuildEpisodeSmartName [FAIL] Assert.Equal() Failure Position: First difference is at position 11 Expected: Programme 5 Actual: Programme 5: Stack Trace: C:\projects\radiodownloader\Test\Classes\TestTextUtils.cs(426,0): at RadioDldTest.TestTextUtils.BuildEpisodeSmartName()

Specifically the test for programme5 which appears to be returning the programmeName plus colon. (this test is passing, actually all 8 tests are passing, when i run the tests locally.) Will have a closer look at this later this week.

[ribbons]

Okay, no worries - I'll wait for your update.

[nbl1268]

ok problem solved. Ready for you to review.

[nbl1268]

Thanks, yes I feel that the regex and trim methods that I've used here 'work' but they are anything but 'elegant'. Separate private function to strip off whitespace and delimiter is good; will work on that.

[ribbons]

Many thanks for your work on this one. I used these commits as the basis for 52da3a9d5fdc3c0464e6cc196b406ae281b4b357 which I've just pushed to master and will be in the next release.