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.

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

Update role models (fix issue #61) #63

Open
wants to merge 15 commits into
base: staging
Choose a base branch
from
Open

Update role models (fix issue #61) #63

wants to merge 15 commits into from

Conversation

KrishnaKanth1729
Copy link

@KrishnaKanth1729 KrishnaKanth1729 commented Oct 7, 2021 •
edited
Loading

Summary

Resolve Issue #61

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, ...)

github-actions[bot]
github-actions bot reviewed Oct 7, 2021
View reviewed changes
Copy link

@github-actions github-actions bot left a comment •
edited by mohamed040406
Loading

Choose a reason for hiding this comment

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

Hey **KrishnaKanth1729**, welcome to the repo for the Tech With Tim API. Please follow the following guidelines while opening a PR:
  • Any new or changed endpoints should be thoroughly documented.
  • Write and or update tests for your new / updated endpoints.
  • All code should be easly readable or commented.

If your code does not meet these requirements your PR will not be accepted.

takos22
takos22 requested changes Oct 7, 2021
View reviewed changes
api/versions/v1/routers/roles/models.py Outdated Show resolved Hide resolved
api/versions/v1/routers/roles/models.py Show resolved Hide resolved
@takos22 takos22 changed the title Resolved Issue #61 Update role models (fix issue #61) Oct 7, 2021
@mohamed040406 mohamed040406 linked an issue Oct 8, 2021 that may be closed by this pull request
[BUG] Role model updates #61
Open
@takos22 takos22 added the bug Something isn't working label Oct 8, 2021
@takos22 takos22 added this to the Initial Release milestone Oct 8, 2021
@KrishnaKanth1729
Copy link
Author

KrishnaKanth1729 commented Oct 14, 2021 •
edited
Loading

Based on the error i guessed that changing the hex ints color values to strings may fix this -> https://github.com/Tech-With-Tim/API/pull/63/checks?check_run_id=3842105193. I am not sure. So i did fb82222

@SylteA
Copy link
Member

SylteA commented Oct 14, 2021

@Tech-With-Tim/project-lead Thoughs on subclassing the pydantic.color.Color class to support ints?
Then we wouldn't have to change the way we handle colors.

@takos22
Copy link
Contributor

takos22 commented Oct 14, 2021

@Tech-With-Tim/project-lead Thoughs on subclassing the pydantic.color.Color class to support ints?

We could just add a validator for colors, to try to use pydantic.color.Color and if it raises a TypeError, we convert the argument to a str then try again with the new argument.

@KrishnaKanth1729
Copy link
Author

KrishnaKanth1729 commented Oct 15, 2021 •
edited
Loading

@takos22 why wait for it to raise a TypeError when we can prevent it at the beginning itself by converting it to a string?

@KrishnaKanth1729
Copy link
Author

And did the conversion from int to string in fb82222 not remove that error?

@KrishnaKanth1729
Copy link
Author

I'm sorry for 2 same commits :cheemsdorime: but it was by accident

@KrishnaKanth1729
Copy link
Author

Can one of you run lints and tests when u are back?

@SylteA SylteA requested a review from a team November 6, 2021 08:44
takos22
takos22 requested changes Nov 6, 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.

Fix the tests

tests/test_roles.py Outdated Show resolved Hide resolved
tests/test_roles.py Outdated Show resolved Hide resolved
takos22
takos22 requested changes Nov 11, 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.

Fix the tests and the database queries

api/versions/v1/routers/roles/routes.py Show resolved Hide resolved
tests/test_roles.py Outdated Show resolved Hide resolved
tests/test_roles.py Show resolved Hide resolved
tests/test_roles.py Outdated Show resolved Hide resolved
mohamed040406
mohamed040406 requested changes Nov 21, 2021
View reviewed changes
api/versions/v1/routers/roles/routes.py Outdated Show resolved Hide resolved
tests/test_roles.py Outdated Show resolved Hide resolved
@mohamed040406
Copy link
Contributor

@KrishnaKanth1729 any updates?

@mohamed040406
Copy link
Contributor

@KrishnaKanth1729 can we get this fixed soon?

@KrishnaKanth1729
Copy link
Author

@KrishnaKanth1729 can we get this fixed soon?

what do we do?

@mohamed040406
Copy link
Contributor

@ KrishnaKanth1729 can we get this fixed soon?

what do we do?

@KrishnaKanth1729 Fix the tests, you can use int in the role response model

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@mohamed040406 mohamed040406 mohamed040406 requested changes

@takos22 takos22 takos22 requested changes

Assignees

@KrishnaKanth1729 KrishnaKanth1729

Labels
bug Something isn't working
Projects
None yet
Milestone
Initial Release
Development

Successfully merging this pull request may close these issues.

[BUG] Role model updates
4 participants
@KrishnaKanth1729 @SylteA @takos22 @mohamed040406