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

Add type hints to auth server app #1216

Merged
merged 8 commits into from
Sep 5, 2022
Merged

Add type hints to auth server app #1216

merged 8 commits into from
Sep 5, 2022

Conversation

sbarrios93
Copy link
Contributor

Description

Added type hints to auth server app on modules services/auth-server/app/app/views.py and services/auth-server/app/app/utils.py

Related to: #450

Checklist

  • I have manually tested my changes and I am happy with the result.
  • The documentation reflects the changes.
  • The PR branch is set up to merge into dev instead of master.
  • I haven't introduced breaking changes that would disrupt existing jobs, i.e. backwards compatibility is maintained.
  • In case I changed the dependencies in any requirements.in I have run pip-compile to update the corresponding requirements.txt.
  • In case I changed one of the services' models.py I have performed the appropriate database migrations (refer to the DB migration docs).
  • In case I changed code in the orchest-sdk I followed its release checklist.
  • In case I changed code in the orchest-cli I followed its release checklist.

@welcome
Copy link

welcome bot commented Aug 23, 2022

🚀 Awesome! Thanks for opening this pull request, it is great to see that you don't mind getting your hands dirty.
✨ We are proud to have you as a contributor, so don't forget to add yourself to the list of contributors at the bottom of the README.md!
🏁 To help getting your pull request across the finish line, make sure to check out our contributor guides.
💬 If you need to connect more synchronously with members of the Orchest community, please feel free to chat with us on our Slack.

@CLAassistant
Copy link

CLAassistant commented Aug 23, 2022

CLA assistant check
All committers have signed the CLA.

…t into auth-server-type-hints

* 'auth-server-type-hints' of github.com:sbarrios93/orchest:
  dont load _typeshed module at runtime
  add types to utils.py and views.py on auth-server
  Bump patch on version
  Improve retry message
Copy link
Contributor

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @sbarrios93! 🎉 I did not do an in-depth review, just left one comment about type union syntax, I think we can upgrade it even if Orchest runs (for now) on Python 3.8.

services/auth-server/app/app/views.py Outdated Show resolved Hide resolved
@yannickperrenet
Copy link
Contributor

Thank you for the contribution @sbarrios93 😎!

@yannickperrenet yannickperrenet added the internal Something that does not affect users directly, e.g. code quality. label Aug 23, 2022
@sbarrios93
Copy link
Contributor Author

Thank you for the contribution @sbarrios93 😎!

Happy to help! Looking forward to keep helping this project grow

@astrojuanlu
Copy link
Contributor

Opened an issue about verifying these static types as part of our CI #1222

Opted to use `typing.Union` still where linebreaks would be needed. Line
breaks using the new `|` syntax is pretty horrible, e.g.:
```python
def foo() -> Tuple[
    int, str
] | Tuple[str, str]:
    pass
```

In my opinion the above is much harder to read then:
```python
def foo() -> Union[
    Tuple[int, str],
    Tuple[str, str],
]:
    pass
```
Copy link
Contributor

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏽

@yannickperrenet yannickperrenet merged commit 1df6e91 into orchest:dev Sep 5, 2022
@astrojuanlu
Copy link
Contributor

Thanks for getting this started @sbarrios93! We took the liberty of finishing it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Something that does not affect users directly, e.g. code quality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants