Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expand GeoJSON filetype detection logic #45

Merged
merged 7 commits into from
Sep 15, 2016
Merged

Expand GeoJSON filetype detection logic #45

merged 7 commits into from
Sep 15, 2016

Conversation

who8mycakes
Copy link
Contributor

Resolving GeoJSON detection issues raised here:
*#44
*#35

Sniffing for crs and features as top-level parameters before looking for type expands the detection for GeoJSON file types.

@mapsam
Copy link
Contributor

mapsam commented Sep 1, 2016

smelloscope

😆

@@ -23,6 +23,7 @@ function sniff(buffer, callback) {

if (header.indexOf('\"tilejson\":') !== -1) return callback(null, 'tilejson');
if ((header.indexOf('\"arcs\":') !== -1) || (header.indexOf('\"objects\":') !== -1)) return callback(null, 'topojson');
if ((header.indexOf('\"crs\":') !== -1) || (header.indexOf('\"features\":') !== -1)) return callback(null, 'geojson');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a sanity check for myself, this says "if crs OR features exists in the string, pass it on as a geojson"? Just taking a look at the TopoJSON specification, which does not specify crs anywhere, but it could be added by a user if they wanted: topojson/topojson-specification#5 - I'm not worried about this use case, though.

@mapsam
Copy link
Contributor

mapsam commented Sep 1, 2016

Looking good @who8mycakes! Just a little nitpicky syntax fix I mentioned for the return statement and this is great. Probably good tagging as 0.5.2.

@sgillies
Copy link

sgillies commented Sep 6, 2016

@who8mycakes looks good to me.

@GretaCB
Copy link

GretaCB commented Sep 15, 2016

Staging looks 👍 . Merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants