search

codeforamerica / address-normalizer

A simple tool that takes a CSV file with addresses and normalizes them.
BSD 3-Clause "New" or "Revised" License
16 stars 4 forks source link

Mike refactoring #5

Closed mholubowski closed 11 years ago

mholubowski commented 11 years ago

Refactor and Unit Tests

  1. Refactored majority of code into many focused methods
  2. Converted main normalize functionality from a method within module, to more extensible class
  3. Began Test::Unit suite
    • Example csv's in _/exampledata
    • to run: ruby address_normalized_test.rb

Gained familiarity with the project while prepping it for expansion. All tests passing

mholubowski commented 11 years ago

@daguar, I just rewrote tests as rspec. Do you have a preference between rspec and test unit?

bensheldon commented 11 years ago

My preference is rspec.

On Thu, Jun 13, 2013 at 2:25 PM, mholubowski notifications@github.com wrote:

@daguar just rewrote tests as rspec. Do you have a preference between rspec and test unit?

Reply to this email directly or view it on GitHub: https://github.com/codeforamerica/address-normalizer/pull/5#issuecomment-19426131

daguar commented 11 years ago

+1 for Rspec, but not a strong preference.

On Jun 13, 2013, at 6:00 PM, Ben Sheldon notifications@github.com wrote:

My preference is rspec.

On Thu, Jun 13, 2013 at 2:25 PM, mholubowski notifications@github.com wrote:

@daguar just rewrote tests as rspec. Do you have a preference between rspec and test unit?

Reply to this email directly or view it on GitHub: https://github.com/codeforamerica/address-normalizer/pull/5#issuecomment-19426131 — Reply to this email directly or view it on GitHub.

bensheldon commented 11 years ago

And also, this is an awesome pull request, btw. Thanks for submitting it! :+1:

daguar commented 11 years ago

Agreed. :+1:

PS, @bensheldon - Mike is one of the Google Summer of Code folks this year. He'll be working out of the office, and I think we're gonna get some great stuff done.

bensheldon commented 11 years ago

@mholubowski can you get this passing in Travis? Looks like it needs some other config stuff

bensheldon commented 11 years ago

We should get this PR merged ASAP so that we can continue building off of it (since it has some major changes)

daguar commented 11 years ago

@bensheldon - Agreed. @mholubowski is continuing work on his own fork, so I want to wait until the dust settles a little bit and we have agreed stability on interface.

Mike, feel free to chime in with your own thoughts.

bensheldon commented 11 years ago

I think we should pull as is and have all development done in feature branches on this repo. 

​Context: I went to fix Travis this morning and realized that I couldn't really do a clean commit with this big merge hanging out. 

​We could also close this pull and pull it into a feature branch on this repo (it just gets hard when the defacto HEAD is in another repo).

On Sat, Jun 22, 2013 at 10:25 AM, daguar notifications@github.com wrote:

@bensheldon - Agreed. @mholubowski is continuing work on his own fork, so I want to wait until the dust settles a little bit and we have agreed stability on interface.

Mike, feel free to chime in with your own thoughts.

Reply to this email directly or view it on GitHub: https://github.com/codeforamerica/address-normalizer/pull/5#issuecomment-19861321

daguar commented 11 years ago

I'm cool with you accepting it. This reminds me of two things:

  1. @dthompson - can you give Mike commit access?
  2. Mike - I was going to send an email about this anyway, but let's do feature branches in git on all improvements.

Ben - would love any broad thoughts on the project you might have.

On Jun 22, 2013, at 2:04 PM, Ben Sheldon notifications@github.com wrote:

I think we should pull as is and have all development done in feature branches on this repo.

​Context: I went to fix Travis this morning and realized that I couldn't really do a clean commit with this big merge hanging out.

​We could also close this pull and pull it into a feature branch on this repo (it just gets hard when the defacto HEAD is in another repo).

On Sat, Jun 22, 2013 at 10:25 AM, daguar notifications@github.com wrote:

@bensheldon - Agreed. @mholubowski is continuing work on his own fork, so I want to wait until the dust settles a little bit and we have agreed stability on interface.

Mike, feel free to chime in with your own thoughts.

Reply to this email directly or view it on GitHub: https://github.com/codeforamerica/address-normalizer/pull/5#issuecomment-19861321 — Reply to this email directly or view it on GitHub.