Conversation
roznawsk
left a comment
roznawsk
left a comment
There was a problem hiding this comment.
Lots of good changes!
I like the improvements of poetry - adding the scripts for testing and checking lint and format.
I wonder if we should be testing every new feature in the SDK's - in this case check the new fields in the HLS component. But I guess we don't have to, since we are already testing it in Jellyfish.
There was a problem hiding this comment.
I think we need this file, although the generated files are in two locations. The tests/support/protos/jellyfish.py should be removed. It should look like this:
There was a problem hiding this comment.
We use python-betterproto for generating python code from definitions.
You can run ./compile_proto.sh script to generate it.
This file seems to be generated by the official proto compiler which I don't think is user friendly.
jellyfish/_notifier.py
Outdated
| message = ServerMessage().parse(msg) | ||
| _which, message = betterproto.which_one_of(message, 'content') | ||
| return message | ||
|
No newline at end of file |
There was a problem hiding this comment.
nitpick: add endline. I guess black would fix it in the next PR anyway
3 participants
Related to: fishjam-dev/fishjam#104
Related to: fishjam-dev/elixir_server_sdk#37
After these changes, tests can be run only inside the docker. That's why a poetry command
ci_testwas added.Tests can be run only inside docker because the jellyfish container can't access services running on the host OS through the
localhostaddress.