street-address-rb / street-address

Detect, and dissect, US Street Addresses in strings.
MIT License
168 stars 85 forks source link

Bring inline with Perl 1.0.4 version #19

Closed skippy closed 9 years ago

skippy commented 9 years ago

hello,

This is a pretty significant rewrite, which brings it inline with the perl version. The main behavior changes are:

besides having a passing perl test suite, @schwern rewrote everything to use regexp. In our informal testing, this improved processing speed by 13x as it wasn't recompiling the regexs every time.

If this is too large a merge, and I understand if it is, please let me know what you would like to see be changed. And we are also able and willing to take over this project and move it forward if you would like.

cheers, Adam

derrek commented 9 years ago

Good timing! I was thinking about updating this code last week. I also want to move to named captures, but that might be a follow up since it'd lose pre ruby 1.9.3 compatibility. Let me take a closer look at this today.

derrek commented 9 years ago

This code isn't compatible with ruby 1.8 either but I think it's simply because of the hash syntax used in the tests.

derrek commented 9 years ago

The first version I can get the tests to pass with this code is MRI 2.1. I haven't used Travis but it looks like you may be attempting to run against 2.0.0. Are the tests passing for you on MRI 2.0.0?

derrek commented 9 years ago

The fix for getting to work against is 2.0 is simple. 2.0 doesn't like

private def blah
end

private def foo
end

I've fixed that locally and will push up a new branch after I've gone through all of this.

skippy commented 9 years ago

AH! I didn't know this was running under circleci. We can definitely get it to work down to ruby 2.0 without much problem, but yes, we would probably lose 1.9.3 and earlier.

Named grouping on regexps are pretty damn nice, from an organizational perspective. I would have to lose them... I can look into 1.9.3 compatibility unless you know for sure that is a no-go. I would lobby to drop 1.9.3 support to keep named groupings :)

I'll fix this up to work under ruby 2.0+

derrek commented 9 years ago

I have it working down to 1.9.3. Named capture works in 1.9.3.

skippy commented 9 years ago

ah, awesome! (and I'm glad about the timing BTW :)

derrek commented 9 years ago

I also set up circleci to run tests through various versions.

derrek commented 9 years ago

So far it's looking good - though I'll likely bump to 2.0 as there's a fair number of subtle differences besides ruby 1.8 no longer working.

skippy commented 9 years ago

+1 to ruby 2.0; 1.9.3 was EOL'ed a few months ago

skippy commented 9 years ago

@derrek let me know if you would like me to make these changes or not...it sounds like you already are, but feel free to punt back to us.

if this PR looks good, I have a few more (one is an optimization; one is a tested Address#to_s, Address#line1, and a new Address#line2.

thanks

derrek commented 9 years ago

My initial thought was to go to 2.0 as well, however, I checked with a fairly large client of the library and one of their systems is using StreetAddress on 1.9.3. The only thing necessary to get it to run on 1.9.3 is adding minitest to the Gemfile since it's not packaged until 2.0+.

skippy commented 9 years ago

@derrek I fixed that in another PR (edit: internal to our branch)

I can bring that over into this PR or I can submit the other PR (edit: to this branch)

derrek commented 9 years ago

I already fixed this stuff https://github.com/derrek/street-address/tree/capscihealth-master