mapbox / shapefile-fairy

I put a zipfile under my pillow and the fairy brought me shapefiles
ISC License
17 stars 6 forks source link

replace all backslashes, #19 #20

Closed mapsam closed 7 years ago

mapsam commented 7 years ago

This updates the sanitizeFileName method to replace ALL backslashes, instead of just instances of \\ double backslashes. Adds two new test fixtures that have \ and \\ names in their files.

Bonus: updating test suite to tape, instead of tap, just to be consistent.

Double bonus: this removes some old node test versions, as well as adds node v6

Triple bonus: adds an "everything" test fixture which can close #1

refs: #19

cc @GretaCB

mapsam commented 7 years ago

Current tests failures are related to missing .index files in shapefiles. Are these actually required? @springmeyer @GretaCB

springmeyer commented 7 years ago

@mapsam - So your new test bundles don't have .index? I think that is okay. I unfortuntely don't remember much more to know what is 100 right. The critical things I recall are:

1 would indicate we must always have .index in the bundle, while 2. would advocate for not. My gut says therefore it would be fine to modify your tests to not need an index. /cc @GretaCB for alternate gut check.

mapsam commented 7 years ago

Worth noting this regex won't dedupe underscores in the case of multiple backslashes together, so this:

city\\data.shp

becomes

city__data.shp

Not a huge deal - but the regex could be improved if we want.

GretaCB commented 7 years ago

These are the required files in shapefile-fairy. My guess is I explicitly validated .index to make sure that unpacker wouldn't throw if an index file was detected.

Shapefile-fairy is the command just before preprocessing, and at this point shapefiles are still zipped and can include index files. perhaps added by the user or if looping back in unpacker as a copy job. So, good to have tests for both with index and without index.

So your new test bundles don't have .index? I think that is okay.

Agreed 👍

mapsam commented 7 years ago

Excellent, thanks for taking a look @GretaCB! I'm going to remove the .index check in the valid fixtures test, and will add a conditional test if the index exists.

mapsam commented 7 years ago

@GretaCB I removed the .index check - since it's not required, I couldn't think of an obvious conditional check.

GretaCB commented 7 years ago

I couldn't think of an obvious conditional check.

Perhaps breaking this group test into a separate single test that also checks for .index, using one of the fixtures that already includes an .index file?

mapsam commented 7 years ago

@GretaCB updated the tests to assert against mandatory files, and a separate test that asserts on a new fixture that includes all possible file types. Let me know what ya think!

GretaCB commented 7 years ago

Looks great! 👍 You :rock: