Skip to content
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

adding configurable indent #198

Merged
merged 1 commit into from Jan 25, 2015
Merged

Conversation

pwittchen
Copy link
Contributor

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
Copy link
Member

CI was broken. Kicking it.

@JakeWharton JakeWharton reopened this Jan 22, 2015
@JakeWharton
Copy link
Member

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

@JakeWharton
Copy link
Member

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

@swankjesse
Copy link
Member

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

By the way: what indent are you using?

@pwittchen
Copy link
Contributor Author

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.

@octylFractal
Copy link
Contributor

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

@JakeWharton
Copy link
Member

@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
Copy link
Member

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

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

@JakeWharton
Copy link
Member

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
Copy link
Contributor Author

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 = " ";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make this private.

@swankjesse
Copy link
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.

@pwittchen
Copy link
Contributor Author

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@swankjesse
Copy link
Member

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

@swankjesse
Copy link
Member

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

@pwittchen
Copy link
Contributor Author

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

swankjesse added a commit that referenced this pull request Jan 25, 2015
@swankjesse swankjesse merged commit 0272b4b into square:master Jan 25, 2015
@swankjesse
Copy link
Member

Woohoo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants