adding configurable indent #198

Merged
merged 1 commit into from Jan 25, 2015

Conversation

4 participants
@pwittchen
Contributor

pwittchen commented Jan 22, 2015

Hi,

I have added configurable indent. It solves issue #106 . Indent can be configured in JavaPoet class (old JavaWriter class). Exemplary usage of the configurable indent is presented in the following code snippet:

new JavaPoet(new Indent().level(4));

In addition, I've added appropriate Unit Tests verifying behavior of the library with new functionality.
I wanted to make code clean and as abstract as possible.

I hope it would be useful for you.

Regards,
Piotr

@JakeWharton

This comment has been minimized.

Show comment
Hide comment
@JakeWharton

JakeWharton Jan 22, 2015

Collaborator

CI was broken. Kicking it.

Collaborator

JakeWharton commented Jan 22, 2015

CI was broken. Kicking it.

@JakeWharton JakeWharton reopened this Jan 22, 2015

@JakeWharton

This comment has been minimized.

Show comment
Hide comment
@JakeWharton

JakeWharton Jan 22, 2015

Collaborator

This is a very heavyweight API for a relatively lightweight concept. JavaWriter just took a string for the indent.

Collaborator

JakeWharton commented Jan 22, 2015

This is a very heavyweight API for a relatively lightweight concept. JavaWriter just took a string for the indent.

@JakeWharton

This comment has been minimized.

Show comment
Hide comment
@JakeWharton

JakeWharton Jan 22, 2015

Collaborator

This also doesn't allow using tabs which someone might want.

Collaborator

JakeWharton commented Jan 22, 2015

This also doesn't allow using tabs which someone might want.

@swankjesse

This comment has been minimized.

Show comment
Hide comment
@swankjesse

swankjesse Jan 22, 2015

Member

Yeah, I think we want to just accept a string: " " or " \t", typically.

By the way: what indent are you using?

Member

swankjesse commented Jan 22, 2015

Yeah, I think we want to just accept a string: " " or " \t", typically.

By the way: what indent are you using?

@pwittchen

This comment has been minimized.

Show comment
Hide comment
@pwittchen

pwittchen Jan 23, 2015

Contributor

You are right. I missed \t which by the way I used as indent in some of past projects.
What do you think about such concept of API below?

new JavaPoet(new Indent().space(2));
new JavaPoet(new Indent().tab(1));
// or
new JavaPoet(new Indent().tab());

Would it be acceptable?
I can easily modify source code on the branch to add something like that.

I didn't implemented something like:

new JavaPoet("  ");
or
new JavaPoet("\t");

because in first case, you don't know how many spaces are there just by looking at the source code and you don't know that this argument is used for indent before checking documentation.
I've added this parameter in constructor, because I wanted to to set indent during creating the object.

Contributor

pwittchen commented Jan 23, 2015

You are right. I missed \t which by the way I used as indent in some of past projects.
What do you think about such concept of API below?

new JavaPoet(new Indent().space(2));
new JavaPoet(new Indent().tab(1));
// or
new JavaPoet(new Indent().tab());

Would it be acceptable?
I can easily modify source code on the branch to add something like that.

I didn't implemented something like:

new JavaPoet("  ");
or
new JavaPoet("\t");

because in first case, you don't know how many spaces are there just by looking at the source code and you don't know that this argument is used for indent before checking documentation.
I've added this parameter in constructor, because I wanted to to set indent during creating the object.

@kenzierocks

This comment has been minimized.

Show comment
Hide comment
@kenzierocks

kenzierocks Jan 23, 2015

Contributor

Is there a valid use case for non-space-or-tab indents?

Contributor

kenzierocks commented Jan 23, 2015

Is there a valid use case for non-space-or-tab indents?

@JakeWharton

This comment has been minimized.

Show comment
Hide comment
@JakeWharton

JakeWharton Jan 23, 2015

Collaborator

@kenzierocks If I hate my coworkers: " /*space*/"

@pwittchen Having a dedicated Indent class isn't appropriate. At the very least it should be an immutable options class which can control multiple factors of how we output code. I'm certain we can think of more than just indent to configure, but starting with only indent is wise.

Collaborator

JakeWharton commented Jan 23, 2015

@kenzierocks If I hate my coworkers: " /*space*/"

@pwittchen Having a dedicated Indent class isn't appropriate. At the very least it should be an immutable options class which can control multiple factors of how we output code. I'm certain we can think of more than just indent to configure, but starting with only indent is wise.

@swankjesse

This comment has been minimized.

Show comment
Hide comment
@swankjesse

swankjesse Jan 24, 2015

Member

I think this is too much API for something so unimportant. What about just this:

JavaPoet javaPoet = new JavaPoet();
javaPoet.setIndent("  ");
Member

swankjesse commented Jan 24, 2015

I think this is too much API for something so unimportant. What about just this:

JavaPoet javaPoet = new JavaPoet();
javaPoet.setIndent("  ");
@JakeWharton

This comment has been minimized.

Show comment
Hide comment
@JakeWharton

JakeWharton Jan 24, 2015

Collaborator

Only downside is that's the only mutable part of the library. That said,
LGTM.
On Jan 23, 2015 7:21 PM, "Jesse Wilson" notifications@github.com wrote:

I think this is too much API for something so unimportant. What about just
this:

JavaPoet javaPoet = new JavaPoet();
javaPoet.setIndent(" ");


Reply to this email directly or view it on GitHub
#198 (comment).

Collaborator

JakeWharton commented Jan 24, 2015

Only downside is that's the only mutable part of the library. That said,
LGTM.
On Jan 23, 2015 7:21 PM, "Jesse Wilson" notifications@github.com wrote:

I think this is too much API for something so unimportant. What about just
this:

JavaPoet javaPoet = new JavaPoet();
javaPoet.setIndent(" ");


Reply to this email directly or view it on GitHub
#198 (comment).

@pwittchen

This comment has been minimized.

Show comment
Hide comment
@pwittchen

pwittchen Jan 24, 2015

Contributor

I've updated code. Now, API looks as @swankjesse proposed:

JavaPoet javaPoet = new JavaPoet();
javaPoet.setIndent("  ");

Drawback of this solution is the fact that indent variable in JavaPoet class is mutable and setIndent() method have to be called before add() method if we want to have different indentation than default.

Contributor

pwittchen commented Jan 24, 2015

I've updated code. Now, API looks as @swankjesse proposed:

JavaPoet javaPoet = new JavaPoet();
javaPoet.setIndent("  ");

Drawback of this solution is the fact that indent variable in JavaPoet class is mutable and setIndent() method have to be called before add() method if we want to have different indentation than default.

@@ -49,7 +49,8 @@
* honors imports, indentation, and deferred variable names.
*/
final class CodeWriter {
- private final String indent = " ";
+ public static final String DEFAULT_INDENT = " ";

This comment has been minimized.

@swankjesse

swankjesse Jan 24, 2015

Member

Please make this private.

@swankjesse

swankjesse Jan 24, 2015

Member

Please make this private.

@@ -256,7 +258,6 @@ private CodeWriter emitType(Object arg) throws IOException {
if (classType.isPrimitive()) return emit(classType.getName());
if (classType.isArray()) return emit("$T[]", classType.getComponentType());
return emitType(ClassName.get(classType));
-

This comment has been minimized.

@swankjesse

swankjesse Jan 24, 2015

Member

Please don't reformat this code.

@swankjesse

swankjesse Jan 24, 2015

Member

Please don't reformat this code.

@@ -28,9 +28,11 @@
@Override public Appendable append(CharSequence charSequence) {
return this;
}
+

This comment has been minimized.

@swankjesse

swankjesse Jan 24, 2015

Member

Please don't reformat this code.

@swankjesse

swankjesse Jan 24, 2015

Member

Please don't reformat this code.

@@ -39,11 +41,13 @@
public final CodeBlock fileComment;
public final String packageName;
public final TypeSpec typeSpec;
+ public final String indent;

This comment has been minimized.

@swankjesse

swankjesse Jan 24, 2015

Member

Indent isn't a property of the JavaFile. It's only a property of the CodeWriter. Please remove this!

@swankjesse

swankjesse Jan 24, 2015

Member

Indent isn't a property of the JavaFile. It's only a property of the CodeWriter. Please remove this!

@swankjesse

This comment has been minimized.

Show comment
Hide comment
@swankjesse

swankjesse Jan 24, 2015

Member

Sorry about being so picky here. The JavaPoet / JavaFile / CodeWriter code is not structured the way we want it to be in order to make manipulating display-stuff in JavaPoet easy.

And that means the advice I gave about adding accessors to JavaPoet makes little sense.

What I want is accessors on JavaPoet, and then to change JavaFile.emit() to take a String indent parameter in addition to the Appendable.

Member

swankjesse commented Jan 24, 2015

Sorry about being so picky here. The JavaPoet / JavaFile / CodeWriter code is not structured the way we want it to be in order to make manipulating display-stuff in JavaPoet easy.

And that means the advice I gave about adding accessors to JavaPoet makes little sense.

What I want is accessors on JavaPoet, and then to change JavaFile.emit() to take a String indent parameter in addition to the Appendable.

@pwittchen

This comment has been minimized.

Show comment
Hide comment
@pwittchen

pwittchen Jan 25, 2015

Contributor

I've updated code according to your comments.
I'm only not sure about implementation of emit(Appendable out) method where I left the comment, but I don't know how to improve this without adding indent field to JavaFile class. Now indent is local variable of emit(Appendable out, String indent) method and toString() method uses emit(Appendable out) method, so output of toString() would be inconsistent with emit(Appendable out, String indent) method.

Contributor

pwittchen commented Jan 25, 2015

I've updated code according to your comments.
I'm only not sure about implementation of emit(Appendable out) method where I left the comment, but I don't know how to improve this without adding indent field to JavaFile class. Now indent is local variable of emit(Appendable out, String indent) method and toString() method uses emit(Appendable out) method, so output of toString() would be inconsistent with emit(Appendable out, String indent) method.

@@ -47,13 +47,17 @@ private JavaFile(Builder builder) {
}
public void emit(Appendable out) throws IOException {
+ emit(out, " "); //TODO(pwittchen): this call is made because of toString() and can be improved

This comment has been minimized.

@swankjesse

swankjesse Jan 25, 2015

Member

Just hardcode toString() to call the emit overload with " ". That's reasonable.

@swankjesse

swankjesse Jan 25, 2015

Member

Just hardcode toString() to call the emit overload with " ". That's reasonable.

This comment has been minimized.

@swankjesse

swankjesse Jan 25, 2015

Member

Just hardcode toString() to call the emit overload with " ". That's reasonable.

@swankjesse

swankjesse Jan 25, 2015

Member

Just hardcode toString() to call the emit overload with " ". That's reasonable.

}
- public CodeWriter(Appendable out, ImmutableMap<ClassName, String> importedTypes) {
+ public CodeWriter(Appendable out, ImmutableMap<ClassName, String> importedTypes, String indent) {

This comment has been minimized.

@swankjesse

swankjesse Jan 25, 2015

Member

nit: reorder parameters 3 and 2 ? When there's overloads that make a parameter optional, that parameter should be last.

@swankjesse

swankjesse Jan 25, 2015

Member

nit: reorder parameters 3 and 2 ? When there's overloads that make a parameter optional, that parameter should be last.

+
+ public JavaPoet setIndent(String indent) {
+ this.indent = indent;
+ return this;

This comment has been minimized.

@swankjesse

swankjesse Jan 25, 2015

Member

Nice.

.build()
.toString();
assertThat(source).isEqualTo(""
- + "// Generated 2015-01-13 by JavaWriter. DO NOT EDIT!\n"
+ + "// Generated 2015-01-13 by JavaPoet. DO NOT EDIT!\n"

This comment has been minimized.

@swankjesse

swankjesse Jan 25, 2015

Member

Nice.

+ + "\tDate madeFreshDate;\n"
+ + "\n"
+ + "\tpublic static void main(String[] args) {\n"
+ + "\t\tSystem.out.println(\"Hello World!\");\n"

This comment has been minimized.

@swankjesse

swankjesse Jan 25, 2015

Member

Nice.

@swankjesse

This comment has been minimized.

Show comment
Hide comment
@swankjesse

swankjesse Jan 25, 2015

Member

Thanks for your patience. Two last nitpicks & I'm eager to merge this.

Member

swankjesse commented Jan 25, 2015

Thanks for your patience. Two last nitpicks & I'm eager to merge this.

@swankjesse

This comment has been minimized.

Show comment
Hide comment
@swankjesse

swankjesse Jan 25, 2015

Member

Also: please https://rebaseandsqua.sh
and do our CLA.

Member

swankjesse commented Jan 25, 2015

Also: please https://rebaseandsqua.sh
and do our CLA.

@pwittchen

This comment has been minimized.

Show comment
Hide comment
@pwittchen

pwittchen Jan 25, 2015

Contributor

I've done the following things:

  • performed 2 updates of the code
    • reordered parameters in constructor of CodeWriter class
    • updated toString() method in JavaFile class and hardcoded indent as you suggested
  • merged changes from square:master branch with pwittchen:configurable_indent branch
  • rebased and squashed changes with http://rebaseandsqua.sh website (now there's single commit with changes for this pull request)
  • pushed changes
  • filled Contributor License Agreement (CLA) form

I hope, now everything is fine :-).

Contributor

pwittchen commented Jan 25, 2015

I've done the following things:

  • performed 2 updates of the code
    • reordered parameters in constructor of CodeWriter class
    • updated toString() method in JavaFile class and hardcoded indent as you suggested
  • merged changes from square:master branch with pwittchen:configurable_indent branch
  • rebased and squashed changes with http://rebaseandsqua.sh website (now there's single commit with changes for this pull request)
  • pushed changes
  • filled Contributor License Agreement (CLA) form

I hope, now everything is fine :-).

this.out = checkNotNull(out);
+ this.indent = checkNotNull(indent);

This comment has been minimized.

@swankjesse

swankjesse Jan 25, 2015

Member

Nice.

swankjesse added a commit that referenced this pull request Jan 25, 2015

@swankjesse swankjesse merged commit 0272b4b into square:master Jan 25, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@swankjesse

This comment has been minimized.

Show comment
Hide comment
@swankjesse

swankjesse Jan 25, 2015

Member

Woohoo!

Member

swankjesse commented Jan 25, 2015

Woohoo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment