Conversation
jtnelson
left a comment
jtnelson
left a comment
There was a problem hiding this comment.
This comments on this PR need to be addressed
|
|
||
| @Override | ||
| public CommandGroup prepare() { | ||
| return new DefaultCommandGroup(); |
There was a problem hiding this comment.
why is this not calling invoke()?
|
|
||
| @Override | ||
| public boolean inTransaction() { | ||
| return invoke("inTransaction").with(); |
There was a problem hiding this comment.
why was this added? Why does the public API need a method to know if its in a transaction? What necessitated this?
| * | ||
| * @return {@code true} if a transaction is active | ||
| */ | ||
| public abstract boolean inTransaction(); |
| @Override | ||
| public List<Object> submit(List<Command> commands) { | ||
| return execute(() -> { | ||
| List<TCommand> tcommands = commands.stream().map(Command::toThrift) |
There was a problem hiding this comment.
We need to be consistent with where java <-> thrift stuff happens. We have Language and JavaThriftBridge the latter of which says it eventually wants to replace others. So this process os translating TCommands, etc at least needs to go in that class
| @Override | ||
| public List<Object> submit(String ccl) { | ||
| return execute(() -> { | ||
| List<TCommandVerb> verbs = extractVerbs(ccl); |
There was a problem hiding this comment.
Rename it to extractVerbsIfNecessary() and rename other methods similarly
| // an attempt was made to bind the variables to different values in a | ||
| // previous evaluation. | ||
| // NOTE: These must always be set before evaluating | ||
| // a line just in case an attempt was made to bind the |
There was a problem hiding this comment.
Why did you change the line lengths. That is unecessary
| // syntax. | ||
| // GH-463: Define each accessible Concourse API | ||
| // method as a Closure directly within Groovy to | ||
| // ensure that they can be called with short syntax. |
There was a problem hiding this comment.
Why did you change the line lengths...
| @@ -27,6 +27,7 @@ | |||
| import java.nio.file.Files; | |||
| import java.nio.file.Path; | |||
| import java.nio.file.Paths; | |||
| import java.util.ArrayList; | |||
| import java.util.List; | |||
| import java.util.Set; | |||
There was a problem hiding this comment.
Overall the diffs in this file are too big. You did not implement this as surgically as you should. You are repeating things like timing. You need to rethink how you can surgically inject the handling for raw CCL while preserving the old behaviour as a fallback. The Diff should not be this large and should not repeat things
|
|
||
| /** | ||
| * A script that parses the concourse.thrift IDL file and generates | ||
| * a Java interface with Concourse-style method names (overloaded |
There was a problem hiding this comment.
Line lengths should go up to 80-83 chars
| @@ -238,7 +238,8 @@ abstract class StatefulConcourseService { | |||
| ++pos; | |||
| } | |||
| if(sb.length() > 1) { | |||
| sb.setLength(sb.length() - 2); | |||
| sb.delete(sb.length() - 2, | |||
There was a problem hiding this comment.
why was this reformatted?
1 participant
No description provided.