Conversation
✅ Deploy Preview for thriving-cassata-78ae72 canceled.
|
|
Shouldn't it be plural, i.e. "clients"? |
|
@samredai good point. |
agorajek
changed the title
shangyian
left a comment
shangyian
left a comment
There was a problem hiding this comment.
LGTM! Only one minor comment on the path rename, but feel free to keep that change if you think it's more consistent.
|
|
||
| @router.get( | ||
| "/client/python/add_materialization/{node_name}/{materialization_name}", | ||
| "/datajunction-clients/python/add_materialization/{node_name}/{materialization_name}", |
There was a problem hiding this comment.
To clarify, I don't think this route needs to change, I meant that the contents instead of (for example) client_code_for_adding_materialization will need to change, since we've updated the client code significantly, like the split between DJClient and DJBuilder. But that change can go into a different PR after we've stabilized the client methods
There was a problem hiding this comment.
@shangyian but then what is this reference to client/python/... ?
There was a problem hiding this comment.
Ah, I guess I didn't think of it as a reference to the directory path, but yeah, interpreting it that way makes sense.
There was a problem hiding this comment.
@agorajek this is just the REST API endpoint that the UI calls to return the example snippets it displays. I don't think it has anything to do with the directory path, it was just a coincidence that there was a /client/python/ directory and this REST endpoint was GET /client/python/....
There was a problem hiding this comment.
Ah I see now. Ok... I'll merge this and fix this later, if needed.
Rename /client to /datajunction-clients.
3 participants
Summary
Just the top level rename.
Test Plan
Also started
docker compose --profile demo upand made sure the UI and Jupyter work.make checkpassesmake testshows 100% unit test coverageDeployment Plan
n/a