(If you are not Steve, please ignore this diff! Evan said I could use secure.phabricator.com for a test diff!)
Details
Details
- Reviewers
- None
!
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
| steve.py | ||
|---|---|---|
| 22 | Yes, you don't really need to worry about the efficiency of this operation - dicts are efficient and your program will scale well past Excel! | |
| 38–45 | This business with the "t" array seems unnecessary. Python lets you do something like this: for line in merchange_category_loaded_csv: merchant_categories[line['Key']] = line['Value'] (There are even more elegant ways of doing this in python using list comprehensions, but let's stick to this for now as it's a little easier conceptually.) | |
| 63 | You can say something like: (year, month, day) = re.split(...) This will make it more legible :). I'm not sure I got the order of year/month/day correct, but hopefully you get the idea. | |
| 65–70 | some commenting about what's going on would be nice, this is pretty confusing logic to me. | |
| 79–82 | This try/except usage seems a little weird. I'd probably just do: spend_amt = float(float_val.replace(',','')and simplify it :) | |
| 111–119 | It's not a terrible idea to put something like this in a separate function for readability. | |
| 113–117 | What's with the numbers 34/17/8? Usually when you have "magic numbers", you want to explain what they are... | |
| 141 | This comment could be better - you should explain why it wouldn't work (that's what I'm wondering as I read it). | |
| 151–152 | I'm not sure what this is doing. Why can't you just take in a single list of dicts - why do you need two? | |