Skip to content

Conversation

@mikegough
Copy link
Contributor

No description provided.

Copy link
Contributor

@nikmolnar nikmolnar left a comment

Choose a reason for hiding this comment

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

This looks great, nice work! Please add a test for this new method. You can use the test for import_netcdf for guidance. Otherwise, just a few minor cleanup items.

See: https://github.com/consbio/python-databasin/blob/master/tests/test_client.py#L153

raise

def create_job(self, name, job_args={}, block=False):
#MG self.build_url(JOB_CREATE_PATH) = https://databasin.org/api/v1/jobs/
Copy link
Contributor

Choose a reason for hiding this comment

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

Please delete this line.


def import_lpk(self, lpk_file):
if lpk_file.endswith('.lpk'):
f = open(lpk_file, 'a+b')
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this isn't written to, it should be opened in read-only:

f = open(lpk_file, 'rb')

else:
raise ValueError('File must be an ArcGIS Layer Package with a .lpk extension')

filename = '{0}.lpk'.format(os.path.splitext(os.path.basename(lpk_file))[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

If lpk_file already ends with .lpk (as enforced on L155), why not just grab the basename?

filename = os.path.basename(lpk_file)

'dataset_type': 'ArcGIS_Native'
}

tmp_job = self.create_job('create_import_job', job_args=tmp_job_args, block=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

tmp_job is a misnomer. It's really an "import" job, but simply job would be fine, too.

@coveralls
Copy link

coveralls commented Jul 26, 2018

Coverage Status

Coverage decreased (-1.5%) to 93.787% when pulling 77fe020 on mikegough:Layer-Package-Imports into fa7817a on consbio:master.

@nikmolnar nikmolnar merged commit 89430b1 into consbio:master Jul 26, 2018
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.

3 participants