Skip to content

Yet another fastapi rewrite PR #49

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.

Sign up for GitHub

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

Jump to bottom
Merged
SylteA merged 16 commits into from
Jun 30, 2021
Merged

Yet another fastapi rewrite PR #49

SylteA merged 16 commits into from
Jun 30, 2021

Conversation

@mohamed040406
Copy link
Contributor

@mohamed040406 mohamed040406 commented Jun 19, 2021 •
edited
Loading

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 June 19, 2021 17:47
SylteA
SylteA requested changes Jun 19, 2021
View reviewed changes
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.

Did it all from my phone so might have missed something.

takos22
takos22 requested changes Jun 19, 2021
View reviewed changes
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.

You forgot to rename a cli command but lgtm

SylteA
SylteA requested changes Jun 20, 2021
View reviewed changes
@mohamed040406 mohamed040406 requested a review from SylteA June 28, 2021 20:12
takos22
takos22 requested changes Jun 29, 2021
View reviewed changes
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.

Nothing big, just a few code improvements that would be great

takos22
takos22 approved these changes Jun 30, 2021
View reviewed changes
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

takos22
takos22 requested changes Jun 30, 2021
View reviewed changes
@SylteA SylteA merged commit 743cc84 into Tech-With-Tim:fastapi-rewrite Jun 30, 2021
@mohamed040406
Copy link
Contributor Author

mohamed040406 commented Jun 30, 2021

t.tag party

@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

Reviewers

@takos22 takos22 takos22 requested changes

@SylteA SylteA Awaiting requested review from SylteA

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Initial Release

Development

Successfully merging this pull request may close these issues.

3 participants

@mohamed040406 @SylteA @takos22