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

Added support for .poly files #65

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kikiminou
Copy link

@kikiminou kikiminou commented Feb 20, 2019

Hello,

I have added basic support for .poly files.

POLY files are supported by Osmosis, mapsplit, osmconvert, osmchange and pbftoosm as a way of defining extraction polygons. See https://wiki.openstreetmap.org/wiki/Osmosis/Polygon_Filter_File_Format

I found useful to view this type of file into Leaflet thanks to Leaflet.FileLayer.

EDIT: jquery is required.

Copy link
Collaborator

@leplatrem leplatrem left a comment

Choose a reason for hiding this comment

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

Thanks for the addition! It looks like a relevant format for Leaflet.FileLayer!

We need to add a few tests here to make sure it's working, and allow future contributors to have confidence when making changes there :)
You'll find examples in the code base, don't hesitate to ask for help...

src/leaflet.filelayer.js Outdated Show resolved Hide resolved
src/leaflet.filelayer.js Outdated Show resolved Hide resolved
}
};
lines.splice(0, 1);
lines.splice(lines.length-1, 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do those two lines? Please add a comment :)

Copy link
Author

Choose a reason for hiding this comment

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

Comments added

current = [];
begin = true;
}
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest that you create a dedicated function poly2geojson() with some unit tests :)

Copy link
Author

Choose a reason for hiding this comment

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

Well, it seems to be for me the most complex part... Do I really need to install nodejs and all thoses things called chai mocha simon happen ? I tryed npm, but I get various sorts of errors I don't understand : like "Unsupported platform for [email protected]: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})"... I'm surprised that Debian Linux 9.6 on x64 is not supported by mocha... and I don't have time to search why all those tangled things don't work. Maybe you have a simple procedure?

Copy link
Author

Choose a reason for hiding this comment

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

Also I'm using a virtualized file system and it doesn't seem compatible with npm. It says:

npm ERR! rofs EROFS: read-only file system, symlink '../acorn/bin/acorn' -> '/media/sf_www/kikiminou/Leaflet.FileLayer/node_modules/.bin/acorn'
npm ERR! rofs Often virtualized file systems, or other file systems
npm ERR! rofs that don't support symlinks, give this error.

Copy link
Author

@kikiminou kikiminou Feb 21, 2019

Choose a reason for hiding this comment

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

Maybe I can simply create the entry poly2geojson in test.filelayer.js and let someone else test it ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have time to search why all those tangled things don't work

I know exactly what you mean. And me neither :)

Maybe you have a simple procedure?

Using nvm is usually helpful to get a recent version of npm working.

Maybe I can simply create the entry poly2geojson in test.filelayer.js and let someone else test it ?

You could write the test and let the Continous Integration run it for you when you push to your branch :) Look, that's the output of your last commit https://travis-ci.org/makinacorpus/Leaflet.FileLayer/builds/496427902

Merging without tests is possible too, if folks from @makinacorpus approve it :)

src/leaflet.filelayer.js Outdated Show resolved Hide resolved
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.

2 participants