search

RelationalAI / rai-sdk-python

The RelationalAI Software Development Kit (SDK) for Python.
Apache License 2.0
17 stars 4 forks source link

Add GET & DELETE /transactions #61

Closed geokollias closed 2 years ago

salamehsameera commented 2 years ago

Thanks George, I think I already added the GET here but it's not merged yet https://github.com/RelationalAI/rai-sdk-python/pull/56 I was waiting on Brad's approval

salamehsameera commented 2 years ago

We can completely disregard mine and consider yours, I just added it for tracking the tickets, I can go ahead and close that one without merging

geokollias commented 2 years ago

Ah sorry Sameera, totally missed #56. This one adds also the delete, exports the names & adds the permissions.

salamehsameera commented 2 years ago

Perfect, thanks. I'll close mine. I just want to point out that it might be better for the reviewers to break down changes in separate PRs instead of having a big one. I would - as a reviewer - prefer to have separate PRs for 1. get 2. delete 3. exporting names 4. adding permissions. It's also better in my opinion to do incremental merges so we can rollback changes that break stuff (if any). But that is just my opinion :)

geokollias commented 2 years ago

Perfect, thanks. I'll close mine. I just want to point out that it might be better for the reviewers to break down changes in separate PRs instead of having a big one. I would - as a reviewer - prefer to have separate PRs for 1. get 2. delete 3. exporting names 4. adding permissions. It's also better in my opinion to do incremental merges so we can rollback changes that break stuff (if any). But that is just my opinion :)

I agree on separate PRs when they make sense. Here the changes are pretty small & trivial, so I thought it's fine. Regarding breaking them, exporting & permissions are tightly coupled to the provided functionality so I'd include them with the actual get or delete functions.