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

Add dataset api support #32

Merged
merged 6 commits into from
Dec 18, 2015
Merged

Add dataset api support #32

merged 6 commits into from
Dec 18, 2015

Conversation

rclark
Copy link
Contributor

@rclark rclark commented Dec 16, 2015

Adds basic dataset api support:

mapbox dataset list
mapbox dataset create
mapbox dataset read-dataset
mapbox dataset delete-dataset
mapbox dataset delete-dataset
mapbox dataset list-features
mapbox dataset read-feature
mapbox dataset put-feature
mapbox dataset delete-feature
mapbox dataset batch-update-feature
mapbox dataset create-tileset

The only deviation from the sdk was naming put-feature rather than update-feature. It seems a little confusing to me to have a method called update-feature where the first line of documentation says, "Insert or update".

Also implements mapbox dataset create-tileset which is an uploads.create call in disguise.

cc @perrygeo @sgillies for any further tips on writing python code!

refs #21

@rclark
Copy link
Contributor Author

rclark commented Dec 17, 2015

per #21 (comment), I would also like to add three "higher-level" functions to mapbox datasets. Objections?

@click.option('--input', '-i', default='-',
help="File containing features to insert, update, and/or delete")
@click.pass_context
def delete_feature(ctx, dataset, puts, deletes, input):
Copy link
Contributor

Choose a reason for hiding this comment

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

s/delete_feature/batch_update_feature/

@perrygeo
Copy link
Contributor

@rclark This looks greats.

A few questions

  • The interface to batch-update-features was a bit confusing to me, particularly with optional positional args for puts and deletes. What do you think about breaking that up into batch-put-features and batch-delete-features?
  • The order of subcommands that you suggest above it nice and logical. But click insists on listing it alphabetically which makes the ordering a bit odd. I haven't found any solutions for this yet but maybe others have some ideas?
  • Should we name it datasets (plural) to be consistent with the developers API page?

The higher-level commands, chunked writing and geojson feature streaming are something we should definitely keep on the radar. For now though, I'm happy with the current implementation as a first cut.

@rclark
Copy link
Contributor Author

rclark commented Dec 17, 2015

I agree that the batch update interface is a little confusing, but I'm inclined to keep it because it is a mirror image of the actual API endpoint. In the higher-ground branch I outlined distinct put-features and delete-features methods which might be more clear and more amenable to streaming from other geojson sources.

👍 to rolling those new methods separately in the near future, perhaps as soon as mapbox/cligj#9 lands and we're in a better place to stream geojson through.

I'm missing something here about order of subcommands. Do you mean that you don't like the fact that mapbox datasets --help lists the subcommands in a different order than I wrote them in the code?

👍 I'll rename to datasets. I think I was thrown by the singular upload command.

@perrygeo
Copy link
Contributor

@rclark

re: ordering, i was just noting that in your original comment, the command order makes sense, going from service level methods, to dataset methods, to feature methods.

mapbox dataset list
mapbox dataset create
mapbox dataset read-dataset
mapbox dataset delete-dataset
mapbox dataset update-dataset
mapbox dataset list-features
mapbox dataset read-feature
mapbox dataset put-feature
mapbox dataset delete-feature
mapbox dataset batch-update-feature
mapbox dataset create-tileset

But click alphabetizes them resulting in a not-very-logical order in the generated help page

batch-update-feature 
create               
create-tileset       
delete-dataset       
delete-feature       
list                 
list-features        
put-feature          
read-dataset         
read-feature         
update-dataset       

It might just be a matter of configuring click to change the ordering but I'm not sure that option exits (haven't found it in the click docs). It's ultimately not a big deal and I could see that some people might actually prefer this ordering.

Good catch on the upload (singular), we should change that as well. Naming things is hard.

@rclark
Copy link
Contributor Author

rclark commented Dec 17, 2015

on the upload (singular), we should change that as well.

.... actually... maybe we shouldn't. mapbox upload is actually a higher-level abstraction over a set of API calls. And in this case I think this isn't a singular noun... its actually a verb or so O_o

mapbox upload rclark.my-map ~/tmp/some-data.geojson

@perrygeo
Copy link
Contributor

maybe we shouldn't. mapbox upload is actually a higher-level abstraction over a set of API calls.

Good point, at the sdk it make sense to mirror the API as closely as possible. But maybe we start diverging from the naming at the CLI where it makes sense. I like the idea of making subcommands more verb-like (geocode instead of geocoding) but it doesn't fit every service. I dunno, I suck at naming things, carry on...

@sgillies
Copy link
Contributor

Naming is hard, for sure. I'd love to get a more unix-y feel to the commands – mapbox datasets ls and mapbox datasets rm instead of mapbox datasets list and mapbox datasets delete-dataset – but it'd be hard to make that work across all the resource-action combos.

Here's a possibility: break up the mapbox-datasets command into mapbox-datasets (list and create), mapbox-dataset (read, update, delete), mapbox-features (list and batch update), and mapbox-feature (read, update, delete). Would solve the order issue at the same time.

@perrygeo
Copy link
Contributor

@rclark We should also document the datasets functionality - aka copy and paste the help into the README.

@rclark
Copy link
Contributor Author

rclark commented Dec 18, 2015

Slapped the docs into readme. How about we merge this? I'm excited to get it integrated with #34

@sgillies
Copy link
Contributor

@rclark I'm going to give feature access one more test drive and will merge in ~1 hour. The package is otherwise (change log etc) ready for a release.

if res.status_code != 204:
raise MapboxCLIException(res.text.strip())

@datasets.command(name="batch-update-feature",
Copy link
Contributor

Choose a reason for hiding this comment

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

name="batch-update-features" plural.

@sgillies
Copy link
Contributor

Here's what I did to test it out

$ mapbox datasets create
$fio cat ~/code/Fiona/tests/data/coutwildrnp.shp | fio collect | jq '{put: .features, delete: []}' | mapbox datasets batch-update-feature ciibvsw8g011ynflz5m91tvht
$mapbox datasets list-features ciibvsw8g011ynflz5m91tvht | geojsonio

geojson io

sgillies added a commit that referenced this pull request Dec 18, 2015
Add dataset api support
@sgillies sgillies merged commit c3ba3b9 into master Dec 18, 2015
@sgillies sgillies mentioned this pull request Dec 18, 2015
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