[GitHub] [tinkerpop] spmallette commented on issue #1158: Tinkerpop 2252 Support Parameterized Groovy Script Transformation

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

spmallette commented on issue #1158: Tinkerpop 2252 Support Parameterized Groovy Script Transformation
URL: https://github.com/apache/tinkerpop/pull/1158#issuecomment-510468155
 
 
   I've had some more time to think about this. I'm having trouble envisioning the complete impact to this change so please forgive me for suggesting changes that might seem like I'm taking this design in circles.
   
   That said, I think that everything works more nicely if the `ScriptTranslator` interface changed and simply became:
   
   ```java
   public interface ScriptTranslator extends Translator<Script, String>
   ```
   
   where `Script` is what you currently have named `ParameterizedScript `. Going forward, `Script` can have parameters or not so:
   
   ```java
   public Optional<Map<String,Object>> getParameters()
   ```
   
   Now, `ParameterizedGroovyTranslator` can go away, and we can just use `GroovyTranslator` which will return a `Script` with or without parameters based on the `TypeTranslator` given to it. I think `DefaultTypeTranslator` should simply be configurable by way of the constructor to extract parameters or not. 
   
   I think that these changes should allow you to get rid of the `ThreadLocal` in `Parameters` hopefully (another thing that was bothering me). It allows existing extensions of `DefaultTypeTranslator` to take advantage of `Parameters` in a way that doesn't make them have to reason about that (i.e. they can just add their custom types for conversion and choose whether they opt to parameterize them or not). I think it also unifies the API more nicely for downstream usage. All of this comes at the cost of a breaking change to our APIs, but I think that's acceptable.
   
   I'm not sure if that's all the changes that need to be done for this to be "ready", but I think I'd need to see them in place before I could say more. At least I'm seeing the bigger picture of how this fits more clearly now. Please let me know what you think.
   
   Also, a couple of administrative notes:
   
   1. Please prefer `git rebase` when bringing in new changes from master to your branch (i see a merge commit on this PR - please remove that when you get a chance - thanks)
   2. Don't worry about travis too much - I tend to keep an eye on it and will let you know if there is a test that won't pass because of a change you made. Please consider using docker to validate your builds in addition to local maven builds as docker will run all tests across all Gremlin Language Variants - `docker/build.sh -t -i` will give you extraordinary confidence that your changes are "good".
   
   Thanks for your patience with this pull request. 

----------------------------------------------------------------
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


With regards,
Apache Git Services