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

Ormar p2 #1

Merged
merged 4 commits into from
Feb 4, 2021
Merged

Ormar p2 #1

merged 4 commits into from
Feb 4, 2021

Conversation

collerek
Copy link

@collerek collerek commented Feb 2, 2021

  • Cleaned ormar db adapter with all up to date ormar features
  • Finished ormar db with OrmarBaseOAuthAccountModel
  • Added test for ormar db and ensure all pass
  • Bumped dependency to ormar >=0.9.0,<0.10.0

So I guess now what is left:

  • More tests including some sample fastapi aplications
  • Add sections regarding ormar to documentation in all relevant parts (like models, oauth etc.)

Let me know what you think about the changes.
Also included some columns in the code.

@paolodina
Copy link
Owner

paolodina commented Feb 3, 2021

Thank you very much for your revision! You did the most of the work (btw, I was stuck on tests)

I will:

  • write some docs along the lines of FastAPI Users SQLAlchemy/MongoDB/Tortoise ORM docs, they share a pretty similar structure
  • tests: for that I'd probably ask to FastAPI Users maintainer when I'll do the PR, but looking at their existing test suite I think the tests you wrote could/should be ok (you also add the add/remove oauth2 account tests)
  • black + flake8 + mypy

Sorry for bothering with some off-topic, but what should I do now? Is it ok to commit the missing parts (basically the docs), squash the commits into one and then submit the PR?

@collerek
Copy link
Author

collerek commented Feb 4, 2021

No need to be sorry! :)

You should merge this one (can be into master since it's your fork anyway).
Pull the changes from master of this fork to your local repo (after merge ofc).
Checkout new branch with descriptive name and add missing docs etc (that way it will be easier for maintainer to see what you want to merge and you can always push to this new branch to update if owner request changes.)
Optionally squash the commits. (Before the push, never squash already pushed commits, you can mess other people git history)
Open pull request from your fork to original repo.

Saw that someone opened an issue for gino, you didn't open one for ormar but since the owner said all backends are welcomed it's should be fine🙂

@collerek
Copy link
Author

collerek commented Feb 4, 2021

Btw i forgot to change version of ormar in tool.flit.metadata.requires-extra in toml file to use version >=0.9.0,<0.10.0 so that should also be updated

@paolodina paolodina merged commit 527feb0 into paolodina:master Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants