Skip to content

Conversation

@mohamed040406
Copy link
Contributor

@mohamed040406 mohamed040406 commented Jul 4, 2021

Summary

Migrate discord OAuth from quart to fastapi

Checklist

  • If endpoints were changed then they have been documented and tested.
    • I have updated the docmentation to reflect the changes.
    • I have updated the tests to support the changes.
  • This PR fixes an issue.
  • This PR adds something new (e.g. new endpoint or parameter).
  • This PR is a breaking change (e.g. endpoint or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

@mohamed040406 mohamed040406 requested a review from SylteA July 4, 2021 11:49
Copy link
Contributor

@takos22 takos22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change validation to pydantic and change some things.

Copy link
Contributor

@takos22 takos22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some doc changes are needed but otherwise lgtm

@takos22
Copy link
Contributor

takos22 commented Jul 7, 2021

And fix the checks

Copy link
Contributor

@takos22 takos22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@click.option("-h", "--host", default="127.0.0.1")
@click.option("-d", "--debug", default=False, is_flag=True)
@click.option("-i", "--initdb", default=False, is_flag=True)
@click.option("-r", "--reload", default=False, is_flag=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could make it a resetdb command, I've found it useful in testing from time to time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be added in another pr

@takos22
Copy link
Contributor

takos22 commented Jul 23, 2021

@mohamed040406 are you planning to do the changes or should someone else do them?

@SylteA
Copy link
Member

SylteA commented Jul 23, 2021

@ mohamed040406 are you planning to do the changes or should someone else do them?

technically anyone can do them 👀

@mohamed040406 mohamed040406 self-assigned this Aug 1, 2021
@mohamed040406 mohamed040406 requested review from SylteA and takos22 August 1, 2021 15:52
Copy link
Member

@SylteA SylteA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@takos22 takos22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm except for import orders

api/app.py Outdated
from utils.response import JSONResponse
from aiohttp import ClientSession
from api import versions
import logging
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ordering isn't logical at all, ordering by length doesn't make sense to me. Order by first party, third party and local packages and in each of them put imports then from ... imports and each of these categories order alphabetically or just use isort.

@mohamed040406 mohamed040406 merged commit 9b5c357 into Tech-With-Tim:fastapi-rewrite Aug 2, 2021
@mohamed040406 mohamed040406 deleted the auth branch August 2, 2021 23:02
@mohamed040406 mohamed040406 added this to the Initial Release milestone Sep 6, 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

Development

Successfully merging this pull request may close these issues.

3 participants