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

Fix errorpone warnings #2399

Merged
merged 1 commit into from
Jun 19, 2018
Merged

Fix errorpone warnings #2399

merged 1 commit into from
Jun 19, 2018

Conversation

nickbabcock
Copy link
Contributor

Problem:

Errorprone warnings during builds are irksome.

Solution:

Fix the errors:

  • Most of them in tests
  • The ones involving generic return types are suppressed (TypeParameterUnusedInFormals) as they are part of our public API and I don't think we're changing that now
  • Switched ConsoleReporterFactory.ConsoleStream enum from containing a non-immutable type (PrintStream) to a function returning that printstream (stdout or stderr)
  • Use Guava's Splitter instead of String.split in the ByteRange and AssetServlet classes
  • errorpone detected that the futures were unused in the JerseyClientIntegrationTest.java test, so I collected them as CompletableFutures using jersey rx client (which is backwards incompatible in 2.27, but I can take care of that 👌 ) -- and thus queried for successful completion.
Result:

No more errorprone warnings! No behavior should be changed.

Copy link
Member

@jplock jplock left a comment

Choose a reason for hiding this comment

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

I noticed that @joschi created a remove guava branch, so maybe we should remove the guava usage?

@nickbabcock
Copy link
Contributor Author

Ah interesting. I'm not sure who to appease in this situation. Using String.split will cause more errorprone warnings and it doesn't seem like those should be suppressed. I, myself, am ambivalent to removing guava.

@isaki
Copy link
Contributor

isaki commented Jun 19, 2018

The link provided in the original post provides this alternative:

String.split(String, int) and setting an explicit 'limit' to -1 to match the behaviour of Splitter.

For my own knowledge, what is the advantage of removing Guava?

@alex-shpak
Copy link
Contributor

@isaki-x Less direct dependencies is better, I believe.

@joschi
Copy link
Member

joschi commented Jun 19, 2018

@isaki-x There have been regular questions about the removal of Guava from Dropwizard on the mailing list and on GitHub, as they have (or had, since https://groups.google.com/forum/#!msg/guava-discuss/rX-QXo-67ZU/gLEvfV4CAwAJ) been breaking backward compatibility quite often.

That imposed problems to people using Dropwizard to implement a Spark application, for example.

With the removal of Guava, this use case would be easier to implement, and with Java 8, there's not too much convenience Guava brings to the table and most changes are pretty mechanical.

I'll open a PR for discussing the removal of Guava from Dropwizard 2.x shortly.

@joschi joschi self-assigned this Jun 19, 2018
@joschi joschi merged commit 47a2e6e into dropwizard:master Jun 19, 2018
@jplock jplock added this to the 1.4.0 milestone Jun 19, 2018
@isaki
Copy link
Contributor

isaki commented Jun 19, 2018

@joschi Thanks for the info! I tend to avoid most of the convenience functions in Guava, but I personally am a fan of Guava's multi maps, tables, and immutable collections. If you aren't using those things, I can see why eliminating such a dependency is ideal. And as @alex-shpak has mentioned, fewer direct dependencies can be a very good thing.

@jplock jplock modified the milestones: 1.4.0, 2.0.0 Jun 22, 2018
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.

5 participants