[GitHub] [tinkerpop] sandszhouSZ commented on issue #1158: Tinkerpop 2252

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

sandszhouSZ commented on issue #1158: Tinkerpop 2252
URL: https://github.com/apache/tinkerpop/pull/1158#issuecomment-509549203
 
 
   I am very very sorry for inadvertently close the PR。
   and the previous pr and suggestion here: https://github.com/apache/tinkerpop/pull/1157
   issue could be found here：https://issues.apache.org/jira/browse/TINKERPOP-2252?filter=-2
   
   some solutions here： 
   1. Please note that we don't have a particular code style in TinkerPop but we do try to not use wildcard imports and we try to final variables where it makes sense to do so.
   **_It is Intellij's default config and i have already fix it_**
   
   2. some class level javadoc here would be good. Please provide a reference to GroovyTranslator and explain the difference between this one and that one. A javadoc link from GroovyTranslator to this class would be nice too.
   **_I have add some javadoc before ParameterizedGroovyTranslator and explain the relation with GroovyTranslator, and i think  link from GroovyTranslator is unnecessary and it could make this relations looks  Complex_**
   
   3. I think that you need to implement this slightly differently. I don't think you want a new parameterizedTranslate(Bytecode) method. I think you just want:
   public ParameterizedGroovyResult translate(Bytecode)
   **_Yes，I'm worried about changing to other files at the beginning. I have modified on your advice。_**
   
   4. t would be nice if this could all be implemented as a simple TypeTranslator because then we wouldn't need this special Translator implementation. Still thinking about how this might all "just work" so I could perhaps suggest more changes after these, but for now, I don't think that the constructor for ParameterizedGroovyTranslator should have a construction that take a simple TypeTranslator:because for it to extract parameters it needs a special kind of TypeTranslator (i.e. one that captures parameters like this one). So, you might need a new ParameterizedTypeTranslator interface to accomplish that. I wish you didn't though. Overall this needs a bit more thought to get t right.
   
   

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