wip: implement extend service and suggest properties

drkane / datasette-reconcile

Adds a reconciliation API endpoint to Datasette, based on the Reconciliation Service API specification.

MIT License

22 stars 6 forks source link

wip: implement extend service and suggest properties #27

Closed nicokant closed 7 months ago

nicokant commented 7 months ago

Hi, this is a work in progress to implement extend service and suggest properties. I still don't know the protocol very well and there are lots of things that needs to be handled, so any feedback is appreciated

Related to #7

drkane commented 7 months ago

This looks awesome, thanks @nicokant !

The only change I would suggest is adding some tests to make sure it's returning what we want to return. There's some examples of tests here: https://github.com/drkane/datasette-reconcile/blob/main/tests/test_reconcile.py - and we can even check the results against the reconciliation api schema here: https://github.com/drkane/datasette-reconcile/blob/main/tests/test_reconcile_schema.py

We'd need to work out the exact contents of the tests - we can keep them pretty simple but maybe starting with something like:

making sure it's in the manifest depending on config
making sure requesting a property gets the right one.
etc

Hopefully doing the tests should help with working out what needs to be handled.

nicokant commented 7 months ago

@drkane I've added some test and handled also the support for https via x-forwarded-proto, I've also opened a issue on datasette to implement support for x-forwarded-proto inside get_absolute_url

drkane commented 7 months ago

Thanks @nicokant - this looks good.

I've had a go at making some further changes. Would you be okay to give me permission to push to this pull request?

The main change I've made is to make it so you don't have to define the properties - it picks them up automatically from the table definition. I think it makes sense to have the extend service active all the time rather than switch it on or off. What do you think?

The other changes I've made are:

change the URL endpoints so they match the ones that are hard-coded in the latest draft of the specification. This feels best for ensuring future compatability. The main change is that reconcile/properties becomes reconcile/extend/propose.
add some initial suggest endpoints
add a couple of tests that make sure the responses match the spec

nicokant commented 7 months ago

Hi, I have some issues in giving you permissions to push to this repository since it's not a user-owned fork, but an organization-owned fork and GitHub seems to not support this scenario. I have to create a new PR with a personal fork, just give a couple of minutes.

I agree with what you say, it's definitely better to automatically pick up properties.

nicokant commented 7 months ago

Opened a new PR with the correct permissions #29