Github user dimas-b commented on a diff in the pull request:

    https://github.com/apache/tinkerpop/pull/899#discussion_r206542682
  
    --- Diff: 
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractEvalOpProcessor.java
 ---
    @@ -222,9 +223,33 @@ protected AbstractEvalOpProcessor(final boolean 
manageTransactions) {
          *                                script evaluation.
          * @param bindingsSupplier A function that returns the {@link 
Bindings} to provide to the
          *                         {@link GremlinExecutor#eval} method.
    +     * @see #evalOpInternal(ResponseHandlerContext, Supplier, 
BindingSupplier)
          */
         protected void evalOpInternal(final Context context, final 
Supplier<GremlinExecutor> gremlinExecutorSupplier,
    --- End diff --
    
    Yeah, I was thinking about that but could not really reach any decision 
myself, so I kept it more accessible. In current code no other classes need to 
call the new method, so I'm ok to make it `private`.
    
    However, if we keep the new `evalOpInternal` method `private` by the same 
logic it might be reasonable to keep the new `makeFrame` method `private` as 
well. If other classes ever need to use it directly it would be under similar 
conditions as calling the new `evalOpInternal` method... and right now other 
classes do not need to do that.
    
    What do you think?


---

Reply via email to