search

cfpb / grasshopper

CFPB's streaming batch geocoder
Creative Commons Zero v1.0 Universal
37 stars 13 forks source link

Refactoring Parser Client #195

Closed hkeeler closed 8 years ago

hkeeler commented 8 years ago

Here it is!...the much anticipated refactorings of the grasshopper-parser client. It finally works, including all unit and (almost all) integration tests...more on that in a minute.

This change is dependent on https://github.com/cfpb/grasshopper-parser/pull/33, so these two PRs should probably be reviewed together. It does not take advantage of the parser's new batch parsing. That was just a bit too much to tackle here. Soon!

There is a bit of general refactoring in here as well. There were a few classes that we so similar that I merged them together, and one or more that were not being used at all, so they got the axe. I also renamed a bit for clarity. I don't think there'll be anything too controversial here.

And finally, this PR has not yet had the latest merged on from master. I'm working on that now. Hopefully, not too many conflicts. Once I have that complete, I'll get the :+1: that it's merge-ready.

One a final note, there was one integration test I could not fix. I was able to fix one compilation error by changing which Flow it used, but I couldn't figure how to fix the final bit of the test. I feel like there's an intermediate step needed in there. Its unclear if this was caused by these change, or if it was pre-existing.

hkeeler commented 8 years ago

Just had to fix one small merge conflict. This is now good-to-go for review.

jmarin commented 8 years ago

@hkeeler Finished reviewing this, just a couple of minor style issues. Not merging yet until we figure out what is going on with https://github.com/cfpb/grasshopper-parser/pull/33 (getting test error there)

hkeeler commented 8 years ago

The parser has been refactored to use "code" instead of "type". I also fixed the weird syntax on parsedInputAddressFlow. Should be good-to-go now.