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

Fix some formatting and typos #17

Merged

orweis merged 1 commit into permitio:master from jsdanielh:master

Sep 8, 2022

Contributor

jsdanielh commented Sep 6, 2022

Fix some formatting and typos in several files.
Rename RpcUtilityMethods's get_proccess_details method to
get_process_details.

Contributor

orweis commented Sep 6, 2022

Hi @jsdanielh !
Thanks for the work here. I'll try to review it all tomorrow, typo fixes look great, there are some type changes I'd need to look more deeply into than a quick skim.

orweis requested changes

View reviewed changes

Contributor

orweis left a comment

Looks great- few comments inline; most not critical, only the float->int change requires further discussion

fastapi_websocket_rpc/websocket_rpc_client.py Outdated

    
                          retry_config (dict): Tenacity.retry config (@see https://tenacity.readthedocs.io/en/latest/api.html#retry-main-api) 

                          default_response_timeout (float): default time in seconds

                          retry_config (dict): Tenacity.retry config (@see https://tenacity.readthedocs.io/en/latest/api.html#retry-main-api)

                          default_response_timeout (int): default time in seconds

Contributor

orweis Sep 7, 2022

default_response_timeout ends up being propagated to asyncio.wait_for - which does accept float ;
So float seems the more correct option, allowing the caller to actually set fractions of a second for the timeout

fastapi_websocket_rpc/websocket_rpc_endpoint.py

-                          simple_websocket = self._serializing_socket_cls(WebSocketSimplifier(websocket, frame_type=self._frame_type))
-                          channel = RpcChannel(self.methods, simple_websocket, sync_channel_id=self._rpc_channel_get_remote_id, **kwargs)
+                          logger.info(f"Client connected", {
+                                      'remote_address': websocket.client})

Contributor

orweis Sep 7, 2022

This seems inconsistent with the other changes here (preferring to have code in a single line)

fastapi_websocket_rpc/websocket_rpc_endpoint.py

                                   data = await simple_websocket.recv()
                                   await channel.on_message(data)
                           except WebSocketDisconnect:
-                              logger.info(f"Client disconnected - {websocket.client.port} :: {channel.id}")
+                              logger.info(
+                                  f"Client disconnected - {websocket.client.port} :: {channel.id}")

Contributor

orweis Sep 7, 2022

This seems inconsistent with the other changes here (preferring to have code in a single line)

fastapi_websocket_rpc/websocket_rpc_endpoint.py

                               await self.handle_disconnect(websocket, channel)
                           except:
                               # cover cases like - RuntimeError('Cannot call "send" once a close message has been sent.')
-                              logger.info(f"Client connection failed - {websocket.client.port} :: {channel.id}")
+                              logger.info(
+                                  f"Client connection failed - {websocket.client.port} :: {channel.id}")

Contributor

orweis Sep 7, 2022

This seems inconsistent with the other changes here (preferring to have code in a single line)

tests/advanced_rpc_test.py

@@ @@ -2,7 +2,8 @@ @@
               import sys
               # Add parent path to use local src as package for tests
-              sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), os.path.pardir)))
+              sys.path.append(os.path.abspath(os.path.join(
+                  os.path.dirname(__file__), os.path.pardir)))

Contributor

orweis Sep 7, 2022

This seems inconsistent with the other changes here (preferring to have code in a single line)

tests/basic_rpc_test.py

@@ @@ -3,7 +3,8 @@ @@
               import sys
               # Add parent path to use local src as package for tests
-              sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), os.path.pardir)))
+              sys.path.append(os.path.abspath(os.path.join(
+                  os.path.dirname(__file__), os.path.pardir)))

Contributor

orweis Sep 7, 2022

This seems inconsistent with the other changes here (preferring to have code in a single line)

tests/binary_rpc_test.py

@@ @@ -3,7 +3,8 @@ @@
               import sys
               # Add parent path to use local src as package for tests
-              sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), os.path.pardir)))
+              sys.path.append(os.path.abspath(os.path.join(

Contributor

orweis Sep 7, 2022

This seems inconsistent with the other changes here (preferring to have code in a single line)

tests/custom_methods_test.py

@@ @@ -13,7 +13,8 @@ @@
               import sys
               # Add parent path to use local src as package for tests
-              sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), os.path.pardir)))
+              sys.path.append(os.path.abspath(os.path.join(

Contributor

orweis Sep 7, 2022

This seems inconsistent with the other changes here (preferring to have code in a single line)

tests/fast_api_depends_test.py

@@ @@ -5,7 +5,8 @@ @@
               from websockets.exceptions import InvalidStatusCode
               # Add parent path to use local src as package for tests
-              sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), os.path.pardir)))
+              sys.path.append(os.path.abspath(os.path.join(

Contributor

orweis Sep 7, 2022

This seems inconsistent with the other changes here (preferring to have code in a single line)

tests/trigger_flow_test.py

@@ @@ -6,7 +6,8 @@ @@
               import sys
               # Add parent path to use local src as package for tests
-              sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), os.path.pardir)))
+              sys.path.append(os.path.abspath(os.path.join(

Contributor

orweis Sep 7, 2022

This seems inconsistent with the other changes here (preferring to have code in a single line)


          Fix some formatting and typos

22e1908

- Fix some formatting and typos in several files.
- Rename `RpcUtilityMethods`'s `get_proccess_details` method to
  `get_process_details`.

jsdanielh force-pushed the master branch from cded488 to 22e1908 Compare

September 7, 2022 17:40

Contributor Author

jsdanielh commented Sep 7, 2022

Looks great- few comments inline; most not critical, only the float->int change requires further discussion

@orweis I applied your suggestion on the type change for the timeout. I actually went and changed it in some other places that were specifying int.
For the line break changes, they go more into complying with PEP8 maximum line specification. Although I know there are some lines that doesn't fully comply with it, I thought a change towards that path would help. Let me know what you think.

orweis approved these changes

View reviewed changes

Contributor

orweis left a comment

Awesomeness. :)
Thank you @jsdanielh

orweis merged commit 9d7bb5a into permitio:master

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