-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add support for lambda expressions #8
Conversation
* @param body the body of the function. | ||
*/ | ||
public Builder addLambda(Iterable<ParameterSpec> parameters, boolean emitTypes, CodeBlock body) { | ||
int paramsLen = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd move these checks into the processing of the parameter stream below. This way we don't go over the parameters list twice. It's small but no reason to double iterate the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed latest. If you agree with the approach i can flag it as resolved, thanks.
* be emitted on the result. | ||
* @param body the body of the function. | ||
*/ | ||
public Builder addLambda(Iterable<ParameterSpec> parameters, boolean emitTypes, CodeBlock body) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wdyt of making emitTypes
an enum instead. Something like public enum LambdaMode ...
. This is a better U/X for users and allows us to add more modes should we ever need to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats sounds like a good improvement and thank your for your comment! Are you thinking something like enum {showTypes;} for type visibility and we can add more values if we need later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed latest, the logic stays pretty much the same, but now we can add various format rules if we need to in the future. When it is needed, we just add a mode to LambdaModes enum and add the case to the parent addLambda method (in CodeBlock). I added 2 new overloads for when the user defines a lambda with no inputs (so he can define a mode, or not and the default one is used) in case a new mode is added in the future that can be applied to said lambdas. Right now, of course, the overloads dont provide discrete value (because when a lambda has no inputs it is moot to specify its input type visibility), but they will seem useful when new modes get added in the LambdaModes enum (that can possible be applied to lambdas with no inputs)
} | ||
|
||
// the body of the lambda (right side) | ||
String bodySide = body.toString().replaceAll("\n", ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems wrong. Why are we doing this? We should not reformat code that has already been specified by users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wdyt about having a new line after the { so for body inputs like:
int x = 6;
int y = 7;
return x + y;
we get outputs like:
input -> {
int x = 6;
int y = 7;
return x + y;
}
(or with proper indents if you think thats better).
By leaving the formatted user input as is, without removing new lines, we might fall into outputs like:
input -> {int x =6;
int y = 7;
return x + y;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The braces placement should be customizable. No one will agree on where to put it. This is why I've been suggesting we have a LambdaSpec
class. I don't see any way around it. If we had a LambdaSpec
builder class we wouldn't need so many method overloads. This would also pay benefits for the switch
PR. the body after the ->
arm of a switch is just like a lambda and a builder would help there.
I'd like to see all these method overloads moved into a builder class, possibly with a bas class that can re-used for the switch PR. The placement of the lambda body braces should be configurable just as it is in ClodeBlock
|
||
// in case of multiple statements, braces are mandatory | ||
if (bodySide.contains(";")) { | ||
bodySide = "{" + bodySide + "}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is forcing a coding style. Users need to be able to specify newlines, indents, etc. Many people will not like the output of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The emitted braces are for lambdas that contain a full method body such as x -> {int y=5; return x + 5;} in contrast with the more common lambdas x -> x + 5 that dont have mutlitple statements. I dont exaclty understand what you mean about forcing a code style, since that braces in the first example are mandatory to producing valid java syntax. Are you saying that the user should be responsible for placing the braces in its code blocks if he wishes to have mutliple statements, so just remove lines 460-462? About the new lines and indents, since the body is a CodeBlock, the user can definitely place indents and new lines in the body if he wishes to, when creating the CodeBlock. Some further insights are very welcome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indenting, newlines, etc. must all be configurable by users.
} | ||
|
||
// the full lambda structure | ||
add(inputSide + " -> " + bodySide); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming spaces around ->
is wrong. Like all things, we must allow users to specify how code is formatted. FYI - this is why I suggested a separate LambdaBlock
class. There are simply too many options to force into method arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea was based on the rest of the codebase that seems to follow that kind of pattern. We have methods such as beginControlFlow() for example that does indeed emitt spaces before { to produce the output and doesnt allow user configuration. I take that this is coded that way because spaces before { on if statements and for loops, are a standard for java code.
3adf8bc
to
6518576
Compare
feature used to structure anonymous functions with CodeBlock and within MethodSpec
@HliasMpGH is there anyone else you know who can help review this? |
fd201f7
to
4478030
Compare
The implementation of the LambdaSpec class can be seen in #16. Closing |
FIRST PART OF ISSUE #5
This feature allows the use of lambda expressions in CodeBlocks and within MethodSpecs.
A usage of the feature can be:
to produce:
The visibility of the input types in the result is a conditional that can be configured as follows:
that produces:
or:
to get:
It is not required to specify if the body of the lambda is an expression or not when using the feature. The output will reflect, depending on the inputs, the wanted and most importantly, valid, result.
if multiple statements are given as a body to the lambda, in the ouput it will be included in braces.