search

GlobalFishingWatch / treniformis

Apache License 2.0
5 stars 6 forks source link

Migrate from SkyTruth/vessel-lists #13

Closed geowurster closed 8 years ago

geowurster commented 8 years ago

Closes https://github.com/GlobalFishingWatch/treniformis/issues/10 Closes https://github.com/GlobalFishingWatch/treniformis/issues/12

This PR started as https://github.com/GlobalFishingWatch/treniformis/pull/1, but the git history there contains a bunch of files we don't want, so I manually copied everything to this branch.

@bitsofbits Ready for final review.

bitsofbits commented 8 years ago

@geowurster, WKV is still around, weren't we going get rid of that?

Also, rather than recopying the branch, to get a clean history, can't we just do a squash commit (https://github.com/blog/2141-squash-your-commits) from the branch to get a clean history?

bitsofbits commented 8 years ago

@geowurster, other than the above things look fine.

My OCD is telling me that we may want to tweak the names again because all of these are MMSI lists, yet only one of the paths uses _MMSI in the designation. The top-level names might be better as ("ACTIVE_MMSI", "FISHING_MMSI", "SPOOFING_MMSI") or even ("MMSI/ACTIVE", "MMSI/FISHING", "MMSI/SPOOFING"). I'd be fine renaming things after the merge so we can think about the structure a bit more first.

geowurster commented 8 years ago

@bitsofbits Both good points. I had forgotten about the squash. You're probably right about the MMSI tag. I think ACTIVE_MMSI is probably better than MMSI/ACTIVE. Not sure how WKV made its way back in, but it should be removed.

bitsofbits commented 8 years ago

@geowurster, I removed WKV and renamed XXX to XXX_MMSI everywhere. I also fixed the resulting breakage of the build_combined_fishing_list and the unit tests.

I think it's good to go. Take a look over the branch when you're back online and if it looks good, please merge it.