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

Excavator: Format Java files #1285

Closed
wants to merge 12 commits into from
Closed

Excavator: Format Java files #1285

wants to merge 12 commits into from

Conversation

svc-excavator-bot
Copy link
Collaborator

excavator is a bot for automating changes across repositories.

Changes produced by the roomba/format check.

To enable or disable this check, please contact the maintainers of Excavator.

.fallbackToCommonNameVerification(
config.fallbackToCommonNameVerification().orElse(DEFAULT_FALLBACK_TO_COMMON_NAME_VERIFICATION))
.proxy(config.proxy().map(ClientConfigurations::createProxySelector).orElseGet(ProxySelector
::getDefault))
Copy link
Contributor

Choose a reason for hiding this comment

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

so.. this is the kind of sadness enabled by the quick hack in palantir/palantir-java-format#44

.failedUrlCooldown(Duration.ofMillis(0))
.build())
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("If nodeSelectionStrategy is ROUND_ROBIN then failedUrlCooldown must be positive");
Copy link
Contributor

Choose a reason for hiding this comment

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

this is much nicer, exactly how I would have formatted it myself!

service,
index,
objectMapper.readValue(jsonString, cls));
method.invoke(service, index, objectMapper.readValue(jsonString, cls));
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

.protocol(Protocol.HTTP_1_1)
.code(code)
.message("unused")
.build();
Copy link
Contributor

Choose a reason for hiding this comment

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

@dansanduleac thoughts on some kind of heuristic that detects over 4 dot method invocations and doesn't try to squish them onto one line?

config.keyStoreType(),
config.keyStoreKeyAlias()).getKeyManagers();
config.keyStorePath().get(), config.keyStorePassword()
.get(), config.keyStoreType(), config.keyStoreKeyAlias())
Copy link
Contributor

Choose a reason for hiding this comment

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

this is quite bad now in 0.3.1

@@ -71,7 +71,8 @@ public void testFromParameters_fillsInDefaults() {
ClientConfiguration actual = ClientConfigurations.of(uris, sslFactory, trustManager);

assertThat(actual.sslSocketFactory()).isInstanceOf(KeepAliveSslSocketFactory.class);
assertThat(((KeepAliveSslSocketFactory) actual.sslSocketFactory()).getDelegate()).isEqualTo(sslFactory);
assertThat(((KeepAliveSslSocketFactory) actual.sslSocketFactory()).getDelegate())
.isEqualTo(sslFactory);
Copy link
Contributor

@dansanduleac dansanduleac Nov 12, 2019

Choose a reason for hiding this comment

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

@iamdanfox @ferozco 0.3.1 made a lot of the fluent assertions line break.
I don't think that's necessarily bad though, it makes it a lot more obvious that there is a method call without having to look all the way to the right for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the additional break is fine

@stale
Copy link

stale bot commented Nov 29, 2019

This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days.

@stale stale bot added the stale label Nov 29, 2019
@stale stale bot removed the stale label Dec 4, 2019
@dansanduleac dansanduleac reopened this Dec 10, 2019
new JacksonDecoder(objectMapper))))))));
return new NeverReturnNullDecoder(new Java8OptionalAwareDecoder(new GuavaOptionalAwareDecoder(
new EmptyContainerDecoder(objectMapper, new InputStreamDelegateDecoder(new TextDelegateDecoder(
new CborDelegateDecoder(cborObjectMapper, new JacksonDecoder(objectMapper))))))));
Copy link
Contributor

Choose a reason for hiding this comment

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

unclear if this is better... but I think it's acceptable

.from(createTestConfig("http://foo-bar-bogus-host.unresolvable:80",
"http://localhost:" + failoverTestCase.server1.getPort()))
.from(createTestConfig("http://foo-bar-bogus-host.unresolvable:80", "http://localhost:"
+ failoverTestCase.server1.getPort()))
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not good but I have an idea of how to fix it

Copy link
Contributor

Choose a reason for hiding this comment

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

@stale
Copy link

stale bot commented Dec 24, 2019

This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days.

@stale stale bot added the stale label Dec 24, 2019
@stale stale bot closed this Dec 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants