Closed btv closed 9 years ago
Hey! Thanks for this.
I think this makes a lot of sense, and the new method you propose makes it prone to less errors in the longer run. Please make the changes to the other parsing functions as you see fit. No rush, take your time. :)
In the meantime, I will forward you the CLA.
Happy Holidays!
Thanks, Bryce. LMK if you would like me to test and merge this separately or if you prefer to have it all done first.
Hey Alex,
Happy Holidays and sorry for the delay in my reply. I'm happy for you to test and merge if you want. I did the best I could to test my change, which I documented in my original MR request. However, if you know a better way to test would you please share that with me?I definitely want to make sure I keep things the same as I do the "remodel".
Warm regards, Bryce
On 12/23/2014 10:25 AM, Alex Pinto wrote:
Thanks, Bryce. LMK if you would like me to test and merge this separately or if you prefer to have it all done first.
— Reply to this email directly or view it on GitHub https://github.com/mlsecproject/combine/pull/98#issuecomment-67982480.
No worries, Bryce. Happy holidays! The test you suggested was ok! What I meant was if you wanted to get a go at the remodel part of the rest of the functions before I merged this.
I'll go ahead and merge this one, and then you can carry on with the changes as you see fit.
Actually, you should compare crop.json
before and after. Just ran a quick test and packetmail is not parsing anything with the new code. Could you check it again, please?
Hey Alex,
I figured out that I was testing against the wrong file. I've verified that my change doesn't actually work so I'm in the process of fixing it. I'll let you know when I have it working.
Sorry about that!
On 12/26/2014 10:39 PM, Alex Pinto wrote:
Actually, you should compare |crop.json| before and after. Just ran a quick test and packetmail is not parsing anything with the new code. Could you check it again, please?
— Reply to this email directly or view it on GitHub https://github.com/mlsecproject/combine/pull/98#issuecomment-68170705.
Fixed! Like the commit message says, I need to use the splitlines method on response. I also pulled out the today functions from within the loops. These changes improved the run time of thresher from 14.5 seconds to 10.5 seconds on my machine.
Nice catch. There should be several small optimizations like this on the code. Tested this and merging soon.
This is less of an actual MR and more about trying to start a discussion about migrating the input parsing to being handled by the csv module instead of being done by hand. Let me know what you think and I can modify the rest of the parseing functiont to do the same.
Testing for this was:
Happy Holidays!