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 post save, edit, delete hooks #210

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ocervell
Copy link

@ocervell ocervell commented Feb 2, 2023

@dantownsend
Copy link
Member

Thanks a lot for this - it's looking good 👍

The tests are failing, but it looks like something pretty minor.

@dantownsend
Copy link
Member

Sorry for the slow reply. The tests should pass now when updated from master.

@ocervell
Copy link
Author

ocervell commented May 8, 2023

Sorry I cannot commit more time to this at the moment. I might come back to it later ...

@sinisaos
Copy link
Member

sinisaos commented May 9, 2023

@ocervell @dantownsend If you both agree, I can make a new PR based on this and fix the linter errors and tests so they can be merged into master (if PR pass review). If not, forget about this comment.

@dantownsend
Copy link
Member

Sorry I cannot commit more time to this at the moment. I might come back to it later ...

@ocervell No problem - I think it's mostly done, just some final tweaks 👍

@sinisaos Yes, that would be helpful, thanks. I think the only outstanding question (I think it was related to this PR, but maybe not) was I think the POST endpoint returns a list containing the ID instead of just the ID. To change it would be a breaking change, but might make more sense.

@sinisaos
Copy link
Member

sinisaos commented May 9, 2023

@sinisaos Yes, that would be helpful, thanks.

OK. I will make a new PR based on this PR with linter errors, some typos in the docs and tests fixed.

I think the only outstanding question (I think it was related to this PR, but maybe not) was I think the POST endpoint returns a list containing the ID instead of just the ID. To change it would be a breaking change, but might make more sense.

No, it's not in this PR or any PR. This is mention in issue #211.
UPDATE:
I just checked and nothing actually breaks (I don't know if you have more complicated projects that could be affected by this change). Only one FastAPI test failed ([{"id": 2}] should be {"id": 2}). Piccolo Admin also works without changes. Hope that helps.

@sinisaos sinisaos mentioned this pull request May 9, 2023
2 tasks
@JoshYuJump
Copy link
Contributor

Any news?

@sinisaos
Copy link
Member

@JoshYuJump There was a #231 based on this PR with fixed linter errors and tests., but I closed it due to lack of interest in it. Feel free to use the code from that PR and redeploy it, if there is interest in implementing it now.

@JoshYuJump
Copy link
Contributor

@JoshYuJump There was a #231 based on this PR with fixed linter errors and tests., but I closed it due to lack of interest in it. Feel free to use the code from that PR and redeploy it, if there is interest in implementing it now.

@sinisaos Thanks your solution, I used the code from the PR, it works fine.

@sinisaos
Copy link
Member

@JoshYuJump Thanks. Yes, PR works and passes all tests and linter checks and is ready to merge. Feel free to use the code from that PR, you might have better luck and your PR will be merged.

P.S. There are a few more PRs (#145, #160, #226) with interesting features that I closed because they were open for too long and I gave up on them. Check them out if you want and feel free to use the code for your PRs (if you're interested in making PRs) as you might have better luck getting them merged. I think they are good and useful and will make the library better.

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.

4 participants