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 typing utils module #151

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add typing utils module #151

wants to merge 4 commits into from

Conversation

DrPyser
Copy link
Contributor

@DrPyser DrPyser commented Sep 20, 2024

  • JSON typing alias

Copy link
Contributor

Build succeeded.
https://zuul.wazo.community/zuul/t/local/buildset/de11ab99e3fd4e48952b43990963fd9c

✔️ tox-linters SUCCESS in 4m 13s
✔️ wazo-tox-py39 SUCCESS in 4m 27s
✔️ wazo-tox-integration-py39 SUCCESS in 3m 24s
✔️ debian-packaging-bullseye SUCCESS in 1m 53s

debian/control Show resolved Hide resolved
xivo/tests/test_typing_utils.py Outdated Show resolved Hide resolved
Copy link
Contributor

Build failed.
https://zuul.wazo.community/zuul/t/local/buildset/619e30717d8a4d918f5e5cb4f47c4cae

✔️ tox-linters SUCCESS in 4m 42s
✔️ wazo-tox-py39 SUCCESS in 4m 36s
✔️ wazo-tox-integration-py39 SUCCESS in 3m 14s
debian-packaging-bullseye FAILURE in 1m 15s

Copy link
Contributor

Build succeeded.
https://zuul.wazo.community/zuul/t/local/buildset/d047e77e86aa4496a4a3a27f239e7adc

✔️ tox-linters SUCCESS in 6m 24s
✔️ wazo-tox-py39 SUCCESS in 7m 02s
✔️ wazo-tox-integration-py39 SUCCESS in 3m 35s
✔️ debian-packaging-bullseye SUCCESS in 1m 56s

@@ -0,0 +1,24 @@
# Copyright 2014-2024 The Wazo Authors (see the AUTHORS file)
Copy link
Member

Choose a reason for hiding this comment

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

why 2014?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. I think I copied it from somewhere? I'll change it.

Copy link
Member

@fblackburn1 fblackburn1 left a comment

Choose a reason for hiding this comment

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

Added mergeit label in the release process
But I don't approve this PR
The usage (for now) is only in python client which should never depend of this library (even only for typing)

@DrPyser
Copy link
Contributor Author

DrPyser commented Sep 30, 2024

Added mergeit label in the release process But I don't approve this PR The usage (for now) is only in python client which should never depend of this library (even only for typing)

We already discussed this previously, sorry that wasn't understood then.

Why is it bad for the clients to depend on xivo-lib-python?

I suppose we should duplicate this module in wazo-lib-rest-client for usage in wazo python clients?

@fblackburn1
Copy link
Member

Why is it bad for the clients to depend on xivo-lib-python?

xivo-lib-python is intended to be a server library, not to be installed outside of the server. We don't want to block ourself/packaging because we add a dependency on wazo python clients (ex: build debian based on requirements.txt, or move requirements.txt to setup.py/pyproject.toml)

Rule of thumb about lib-python, when it doesn't make sense or if the code is huge, make a separate module. IMHO we are in the first scenario

I suppose we should duplicate this module in wazo-lib-rest-client for usage in wazo python clients?

Yes it's the easier solution (and probably the one that is worth it)
Another solution for this specific library, is to make a generic typing library + debian packaging (if the python library already exist, then it's to make debian packaging). To be determined if it worth it ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants