[GitHub] [tinkerpop] spmallette commented on a change in pull request #1280: gremlin-python: fix up serializer out of range of int

[GitBox <gi...@apache.org>]

spmallette commented on a change in pull request #1280:
URL: https://github.com/apache/tinkerpop/pull/1280#discussion_r411321142



##########
File path: gremlin-python/src/main/jython/tests/driver/test_client.py
##########
@@ -129,6 +129,17 @@ def test_multi_conn_pool(client):
     assert len(result_set.all().result()) == 6
 
 
+def test_client_bytecode_with_int(client):
+    g = Graph().traversal()
+    t = g.V().has('age', 851401972585122).count()

Review comment:
       Thus far we've forced python users to be explicit and think in java terms, thus:
   
   ```python
   g.V().has('age', long(851401972585122)).count()
   ```
   
   but I suppose it's better if this were just handled automatically as it might yield less confusion and be more pythonic.  The test you added here is fine but please add a test here:
   
   https://github.com/apache/tinkerpop/blob/3.3-dev/gremlin-python/src/main/jython/tests/structure/io/test_graphsonV2d0.py#L306
   
   and here:
   
   https://github.com/apache/tinkerpop/blob/3.3-dev/gremlin-python/src/main/jython/tests/structure/io/test_graphsonV3d0.py#L357
   
   to more directly test GraphSON. Please do not remove the existing tests that use the old `long()` syntax as I think it's still worth testing that since it's something we will continue to support.  Just add new assertions for your new logic. I guess it wouldn't hurt to add tests to cover both positive and negative values in your range.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org