search

qri-io / rfcs

Request For Comments (RFCs) documenting changes to Qri
MIT License
12 stars 6 forks source link

Adding geospatial functionality to Skylark std lib (WIP) #21

Closed stuartlynn closed 5 years ago

stuartlynn commented 6 years ago

This is still a work in progress but wanted to make sure there was eyes on to make sure the level of detail and contents are what you are looking for in one of these RFCS.

Feel free to give feedback. Aim to have a finished first pass by the end of the week.

stuartlynn commented 6 years ago

Hey @dustmop sorry for the delay in getting back to this. I fleshed out the example and brought the references in line with what I found in the docs.

One thing I am still a little unclear on though. Is the dataset that the download function is passed the original dataset in Qri? If so, then in the case when you want to combine it with an external dataset that you grab over http, is this a valid way to get them both to the transform function?

https://github.com/qri-io/rfcs/pull/21/commits/f2cb4b53dc63755cad6ca704c8c53169361479ab#diff-e0e9e1cec328db974d11dc273dd1209dR42

dustmop commented 6 years ago

@stuartlynn So, the interface we define for starlark functionality is still very much in flux. We plan on changing things in the near future so that download doesn't have any input parameters at all; the idea being that download cannot be used in any way to "leak" data out of datasets that exist in qri. Instead, it's only usable to act as a scraper that retrieves content from the web, and then pipes that result into transform function (which might also change its name soon), at which point the original dataset "body" would be available. Exactly how this piping happens is something we currently discussing, and should have an rfc for soon.

I'll take a look at these new changes tomorrow, sorry!

b5 commented 6 years ago

Annnnnd as @dustmop mentioned, here's the RFC that outlines the proposed changes. Would love your feedback @stuartlynn!

b5 commented 6 years ago

oops, forgot the link: #24

stuartlynn commented 6 years ago

Sounds good! Will leave some comments on that RFC. Let me know what feedback you have for this RFC. Keen to start trying to implement :-)

b5 commented 5 years ago

Ok this is approved and looks great. I'm going to merge, and we can make the minor edits @dustmop outlined with another PR.

Thanks so much for taking the time to write this @stuartlynn!