Skip to content

Update role models (fix issue #61) - #63

Open
KrishnaKanth1729 wants to merge 15 commits into
Tech-With-Tim:stagingfrom
KrishnaKanth1729:staging
Open

Update role models (fix issue #61)#63
KrishnaKanth1729 wants to merge 15 commits into
Tech-With-Tim:stagingfrom
KrishnaKanth1729:staging

Conversation

@KrishnaKanth1729

@KrishnaKanth1729KrishnaKanth1729 commented Oct 7, 2021•
edited
Loading

Copy link
Copy Markdown

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-actionsBot reviewed Oct 7, 2021
View reviewed changes

@github-actionsgithub-actionsBot left a comment •
edited by mohamed040406
Loading

Copy link
Copy Markdown

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 suggested changes Oct 7, 2021
View reviewed changes
Comment threadapi/versions/v1/routers/roles/models.py Outdated
Comment threadapi/versions/v1/routers/roles/models.py
@takos22takos22 changed the title Resolved Issue #61Update role models (fix issue #61)Oct 7, 2021
@mohamed040406mohamed040406 linked an issue Oct 8, 2021 that may be closed by this pull request
[BUG] Role model updates #61
Open
@takos22takos22 added the bug Something isn't working label Oct 8, 2021
@takos22takos22 added this to the Initial Release milestone Oct 8, 2021
@KrishnaKanth1729

KrishnaKanth1729 commented Oct 14, 2021•
edited
Loading

Copy link
Copy Markdown
Author

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

SylteA commented Oct 14, 2021

Copy link
Copy Markdown
Contributor

@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

takos22 commented Oct 14, 2021

Copy link
Copy Markdown
Contributor

@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

KrishnaKanth1729 commented Oct 15, 2021•
edited
Loading

Copy link
Copy Markdown
Author

@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

KrishnaKanth1729 commented Oct 15, 2021

Copy link
Copy Markdown
Author

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

@KrishnaKanth1729

KrishnaKanth1729 commented Oct 15, 2021

Copy link
Copy Markdown
Author

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

@KrishnaKanth1729

KrishnaKanth1729 commented Oct 15, 2021

Copy link
Copy Markdown
Author

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

@SylteA
SylteA requested a review from a teamNovember 6, 2021 08:44
takos22
takos22 suggested changes Nov 6, 2021
View reviewed changes

@takos22takos22 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fix the tests

Comment threadtests/test_roles.py
Comment threadtests/test_roles.py Outdated
takos22
takos22 suggested changes Nov 11, 2021
View reviewed changes

@takos22takos22 left a comment

Copy link
Copy Markdown
Contributor

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

Comment threadapi/versions/v1/routers/roles/routes.py
Comment threadtests/test_roles.py Outdated
Comment threadtests/test_roles.py
Comment threadtests/test_roles.py
mohamed040406
mohamed040406 suggested changes Nov 21, 2021
View reviewed changes
Comment threadapi/versions/v1/routers/roles/routes.py Outdated
Comment threadtests/test_roles.py Outdated
@mohamed040406

mohamed040406 commented Nov 26, 2021

Copy link
Copy Markdown
Contributor

@KrishnaKanth1729 any updates?

@mohamed040406

mohamed040406 commented Feb 23, 2022

Copy link
Copy Markdown
Contributor

@KrishnaKanth1729 can we get this fixed soon?

@KrishnaKanth1729

KrishnaKanth1729 commented Feb 23, 2022

Copy link
Copy Markdown
Author

@KrishnaKanth1729 can we get this fixed soon?

what do we do?

@mohamed040406

mohamed040406 commented Feb 23, 2022

Copy link
Copy Markdown
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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actionsgithub-actions[bot]github-actions[bot] left review comments
+2 more reviewers
@mohamed040406mohamed040406mohamed040406 requested changes
@takos22takos22takos22 requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@KrishnaKanth1729KrishnaKanth1729

Labels

bugSomething 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