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

optional postgress database #322 #370

Closed
wants to merge 4 commits into from

Conversation

steve7158
Copy link
Contributor

@glaslos @afeena please check this PR if it is relevant to issue #322

@glaslos
Copy link
Member

glaslos commented Mar 6, 2020

Please adhere to PEP8 and I'd recommend to not use an ORM. Also I'd like to see more than just a database connection 😉

@steve7158
Copy link
Contributor Author

Ok ill update you as soon as possible:+1:.

@afeena
Copy link
Collaborator

afeena commented Mar 6, 2020

@steve7158 please use async for your implementation :)

@steve7158
Copy link
Contributor Author

Ok, @afeena ill do that 😃 .

@steve7158
Copy link
Contributor Author

steve7158 commented Mar 7, 2020

@afeena, @glaslos its far from complete let me know if i am moving in a right track or not. 😄

@steve7158
Copy link
Contributor Author

also please let me know if it is a good practice or not to make PR commits that are incomplete. I thought it of as a way to review my code. Thank you

@glaslos
Copy link
Member

glaslos commented Mar 7, 2020

It's fine and recommended if you work on a project for the first time. Now you already got important feedback that helps you preparing a solid PR

@steve7158
Copy link
Contributor Author

I see thank you @glaslos

@steve7158 steve7158 closed this Mar 10, 2020
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.

3 participants