aws-cloudformation / cloudformation-resource-schema

The CloudFormation Resource Schema defines the shape and semantic for resources provisioned by CloudFormation. It is used by provider developers using the CloudFormation RPDK.
Apache License 2.0
90 stars 38 forks source link

Adjust auto-formatter settings to not join lines and re-flow comments #68

Closed vladtsir closed 4 years ago

vladtsir commented 4 years ago

Issue #, if available:

Description of changes: I propose that automatic formatter should not reflows comments and join lines together.

If I write something like this:

Schema = Schema.builder()
        .withDraftV7()
        .withMetaSchema(schema1)
        .withMetaSchema(schema2)
        .withSomeOtherOption(true)
        .build();

I do not want it to be changed to

Schema = Schema.builder().withDraftV7().withMetaSchema(schema1).withMetaSchema(schema2).withSomeOtherOption(true).build();

The same goes for comments. I usually put some thought into where the line breaks should go in the comments; formatter usually makes it worse and never makes it better

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

rjlohan commented 4 years ago

Yeah I think leave the line length at 80, the rest is OK.

vladtsir commented 4 years ago

Yeah I think leave the line length at 80, the rest is OK.

this was for the width of comments. max line width was not changed in this CR - had been at 130 all along - ln 311:

<setting id="org.eclipse.jdt.core.formatter.lineSplit" value="130"/>

If I set both to 80 then 7 classes will be re-formatted:

src/main/java/software/amazon/cloudformation/resource/ResourceTypeSchema.java
src/main/java/software/amazon/cloudformation/resource/SchemaValidator.java
src/main/java/software/amazon/cloudformation/resource/Validator.java
src/main/java/software/amazon/cloudformation/resource/exceptions/ValidationException.java
src/test/java/software/amazon/cloudformation/resource/ResourceTypeSchemaTest.java
src/test/java/software/amazon/cloudformation/resource/ValidatorTest.java
src/test/java/software/amazon/cloudformation/resource/exceptions/ValidationExceptionTest.java

should I do it?

rjlohan commented 4 years ago

Post the diff as an extra commit and we'll see what it looks like?

tobywf commented 4 years ago

sorry, didn't realize it was only for comments - can set that to 130, too then. either way is fine.

vladtsir commented 4 years ago

I think 80 is a bit narrow. What if I want to have more than 15 nested loops?

rjlohan commented 4 years ago

I think 80 is a bit narrow. What if I want to have more than 15 nested loops?

You have more problems than line width at that point... but yeah 80 chars does create some crappy line breaks in that diff. @tobywf what do you think?

tobywf commented 4 years ago

happy to revert the last two and leave as is for now. don't want to get bogged down in this, and it's already at 130. thanks to the auto-formatter, can change it whenever we want in future anyway.

it's a balance. 80 is defo short, and can feel pointless on a huge screen. at the same time, you and me do a lot of reviews in meetings where we don't have external monitors. but it seems to have been working fine so far.