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

Use whosonfirst downloader #37

Merged
merged 2 commits into from Oct 17, 2017

Conversation

Projects
None yet
3 participants
@orangejulius
Member

orangejulius commented Sep 29, 2017

Instead of having a second script to download data in this repository,
the one in whosonfirst can be reused, since the NPM package is an
indirect dependency of this package.

The README is updated with usage and a link to the whosonfirst readme
for even more configuration options.

Use whosonfirst downloader
Instead of having a second script to download data in this repository,
the one in whosonfirst can be reused, since the NPM package is an
indirect dependency of this package.

The README is updated with usage and a link to the whosonfirst readme
for even more configuration options.

@orangejulius orangejulius self-assigned this Sep 29, 2017

doesn't work as described

@trescube

This doesn't work since this project doesn't include the pelias-whosonfirst dependency so the downloader isn't available.

@orangejulius

This comment has been minimized.

Show comment
Hide comment
@orangejulius

orangejulius Oct 11, 2017

Member

@trescube Hmmm. but it is an indirect dependency (pelias-wof-admin-lookup depends on pelias-whosonfirst). It worked for me on NPM 5.4.1 (and whatever my other computer is using, I think NPM 3). What version of NPM did you run it with?

The approach of calling node node_modules/... may be flawed. I've heard the NPM team talk about how you shouldn't expect anything within node_modules to be in a particular place.

Member

orangejulius commented Oct 11, 2017

@trescube Hmmm. but it is an indirect dependency (pelias-wof-admin-lookup depends on pelias-whosonfirst). It worked for me on NPM 5.4.1 (and whatever my other computer is using, I think NPM 3). What version of NPM did you run it with?

The approach of calling node node_modules/... may be flawed. I've heard the NPM team talk about how you shouldn't expect anything within node_modules to be in a particular place.

@trescube

This comment has been minimized.

Show comment
Hide comment
@trescube

trescube Oct 11, 2017

Contributor
[stephenhess@slumlord pip-service (master $)]$ npm --version
2.15.11
Contributor

trescube commented Oct 11, 2017

[stephenhess@slumlord pip-service (master $)]$ npm --version
2.15.11
@trescube

This comment has been minimized.

Show comment
Hide comment
@trescube

trescube Oct 11, 2017

Contributor

Here are my contents of node_modules:

[stephenhess@slumlord pip-service (master $)]$ ll node_modules
total 0
drwxr-xr-x    9 stephenhess  staff    306 Oct 11 10:14 express
drwxr-xr-x   11 stephenhess  staff    374 Oct 11 10:14 jshint
drwxr-xr-x  640 stephenhess  staff  21760 Oct 11 10:14 lodash
drwxr-xr-x    8 stephenhess  staff    272 Oct 11 10:14 morgan
drwxr-xr-x    8 stephenhess  staff    272 Oct 11 10:14 npm-check
drwxr-xr-x   12 stephenhess  staff    408 Oct 11 10:14 pelias-logger
drwxr-xr-x   10 stephenhess  staff    340 Oct 11 10:14 pelias-mock-logger
drwxr-xr-x   16 stephenhess  staff    544 Oct 11 10:14 pelias-wof-admin-lookup
drwxr-xr-x    9 stephenhess  staff    306 Oct 11 10:14 precommit-hook
drwxr-xr-x   13 stephenhess  staff    442 Oct 11 10:14 proxyquire
drwxr-xr-x   10 stephenhess  staff    340 Oct 11 10:14 request
drwxr-xr-x    7 stephenhess  staff    238 Oct 11 10:14 semantic-release
drwxr-xr-x    7 stephenhess  staff    238 Oct 11 10:14 tap-dot
drwxr-xr-x   13 stephenhess  staff    442 Oct 11 10:14 tape
drwxr-xr-x   14 stephenhess  staff    476 Oct 11 10:14 temp
drwxr-xr-x    9 stephenhess  staff    306 Oct 11 10:14 through2
Contributor

trescube commented Oct 11, 2017

Here are my contents of node_modules:

[stephenhess@slumlord pip-service (master $)]$ ll node_modules
total 0
drwxr-xr-x    9 stephenhess  staff    306 Oct 11 10:14 express
drwxr-xr-x   11 stephenhess  staff    374 Oct 11 10:14 jshint
drwxr-xr-x  640 stephenhess  staff  21760 Oct 11 10:14 lodash
drwxr-xr-x    8 stephenhess  staff    272 Oct 11 10:14 morgan
drwxr-xr-x    8 stephenhess  staff    272 Oct 11 10:14 npm-check
drwxr-xr-x   12 stephenhess  staff    408 Oct 11 10:14 pelias-logger
drwxr-xr-x   10 stephenhess  staff    340 Oct 11 10:14 pelias-mock-logger
drwxr-xr-x   16 stephenhess  staff    544 Oct 11 10:14 pelias-wof-admin-lookup
drwxr-xr-x    9 stephenhess  staff    306 Oct 11 10:14 precommit-hook
drwxr-xr-x   13 stephenhess  staff    442 Oct 11 10:14 proxyquire
drwxr-xr-x   10 stephenhess  staff    340 Oct 11 10:14 request
drwxr-xr-x    7 stephenhess  staff    238 Oct 11 10:14 semantic-release
drwxr-xr-x    7 stephenhess  staff    238 Oct 11 10:14 tap-dot
drwxr-xr-x   13 stephenhess  staff    442 Oct 11 10:14 tape
drwxr-xr-x   14 stephenhess  staff    476 Oct 11 10:14 temp
drwxr-xr-x    9 stephenhess  staff    306 Oct 11 10:14 through2
@orangejulius

This comment has been minimized.

Show comment
Hide comment
@orangejulius

orangejulius Oct 11, 2017

Member

Ah yes that's why. NPM 2 has nested directories. You should seriously upgrade to at least NPM 3. But NPM 5 works pretty well now and is 10X FASTER

Member

orangejulius commented Oct 11, 2017

Ah yes that's why. NPM 2 has nested directories. You should seriously upgrade to at least NPM 3. But NPM 5 works pretty well now and is 10X FASTER

@orangejulius

This comment has been minimized.

Show comment
Hide comment
@orangejulius

orangejulius Oct 11, 2017

Member

NPM 2 is beyond old, and we've explicitly upgraded to 3 in all our production environments. We should bump that up higher too honestly.

Member

orangejulius commented Oct 11, 2017

NPM 2 is beyond old, and we've explicitly upgraded to 3 in all our production environments. We should bump that up higher too honestly.

@trescube

This comment has been minimized.

Show comment
Hide comment
@trescube

trescube Oct 11, 2017

Contributor

I'll update. Could you specify the npm version in engines in package.json?

Contributor

trescube commented Oct 11, 2017

I'll update. Could you specify the npm version in engines in package.json?

@orangejulius

This comment has been minimized.

Show comment
Hide comment
@orangejulius

orangejulius Oct 11, 2017

Member

good idea.

Member

orangejulius commented Oct 11, 2017

good idea.

Add NPM 3 to engines
This repository calls a script within one of its (indirect)
dependencies, and the location of this script changes with NPM 3. In the
future we also need to find a better way to do this. NPX can probably
help.
@trescube

This comment has been minimized.

Show comment
Hide comment
@trescube

trescube Oct 11, 2017

Contributor

Downloader works for me now

Contributor

trescube commented Oct 11, 2017

Downloader works for me now

@orangejulius orangejulius merged commit 79e5ae2 into master Oct 17, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in review label Oct 17, 2017

@orangejulius orangejulius deleted the wof-downloader branch May 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment